i386/89296: Spurious atapci1: failed to enable memory mapping! on ICH7

[Arthur Hartwig]

The following reply was made to PR i386/89296; it has been noted by GNATS.

From: Arthur Hartwig <arthur.hartwig at nokia.com>
To: bug-followup at FreeBSD.org, Francis.Dupont at enst-bretagne.fr
Cc:  
Subject: Re: i386/89296: Spurious atapci1: failed to enable memory mapping!
 on ICH7
Date: Tue, 22 Nov 2005 16:51:40 +1000

 I also saw this on my Gigabyte GA-*i945P-G motherboard which has an ICH7.
 
 The message is printed as a result of a bus_alloc_resource_any() call in 
 ata_intel_chipinit() in ata-chipset.c. The offending call is
 
 /* SATA parts can be either compat or AHCI */
     else {
         /* if we have BAR(5) as a memory resource we should use AHCI mode */
         ctlr->r_type2 = SYS_RES_MEMORY;
         ctlr->r_rid2 =PCIR_BAR(5);
         if ((ctlr->r_res2 = bus_alloc_resource_any(dev, ctlr->r_type2,
                                                    &ctlr->r_rid2, 
 RF_ACTIVE))) {
 
 The bus_alloc_resource_any() call winds up in pci_alloc_resource() in 
 dev/pci/pci.c which sees the RF_ACTIVE flag is set and triies to enable 
 memory space access by calling PCI_ENABLE_IO() to set the appropriate 
 bit in the PCIR_COMMAND  register of the ATA controller. Unfortunately 
 the memory space enable bit in the ICH7 ATA controller on my motherboard 
 is hardwired to 0 unless the SCRAE bit is one. So the ATA driver could 
 be changed to stop this message appearing but I think the logic in 
 pci_alloc_resource() is flawed - it should not be attempting to enable 
 memory or i/o access UNTIL it has successfully allocated the requested 
 resource. Suggested fix:
 
 change pci_alloc_resource() to read as follows:
 struct resource *
 pci_alloc_resource(device_t dev, device_t child, int type, int *rid,
                    u_long start, u_long end, u_long count, u_int flags)
 {
         struct pci_devinfo *dinfo = device_get_ivars(child);
         struct resource_list *rl = &dinfo->resources;
         struct resource_list_entry *rle;
         pcicfgregs *cfg = &dinfo->cfg;
 
         /*
          * Perform lazy resource allocation
          */
         if (device_get_parent(child) == dev) {
                 switch (type) {
                 case SYS_RES_IRQ:
                         /*
                          * If the child device doesn't have an
                          * interrupt routed and is deserving of an
                          * interrupt, try to assign it one.
                          */
                         if (!PCI_INTERRUPT_VALID(cfg->intline) &&
                             (cfg->intpin != 0))
                                 pci_assign_interrupt(dev, child, 0);
                         break;
                 case SYS_RES_IOPORT:
                 case SYS_RES_MEMORY:
                         rle = resource_list_find(rl, type, *rid);
                         if (rle == NULL)
                                 return (pci_alloc_map(dev, child, type, rid,
                                     start, end, count, flags));
                         break;
                 }
                 /*
                  * If we've already allocated the resource, then
                  * return it now.  But first we may need to activate
                  * it, since we don't allocate the resource as active
                  * above.  Normally this would be done down in the
                  * nexus, but since we short-circuit that path we have
                  * to do its job here.  Not sure if we should free the
                  * resource if it fails to activate.
                  */
                 rle = resource_list_find(rl, type, *rid);
                 if (rle != NULL && rle->res != NULL) {
                         if (bootverbose)
                                 device_printf(child,
                             "Reserved %#lx bytes for rid %#x type %d at 
 %#lx\n",
                                     rman_get_size(rle->res), *rid, type,
                                     rman_get_start(rle->res));
                         /*
                          * Enable IO mode only if the matching resource is
                          * successfully activated else the PCI_ENABLE_IO 
 might
                          * fail and report the failre when it doesn't 
 matter.
                          */
                         if (flags & RF_ACTIVE) {
                             if (bus_generic_activate_resource(dev, 
 child, type,
                                  *rid, rle->res) != 0)
                                 return NULL;
                             else
                                 PCI_ENABLE_IO(dev, child, type);
                         }
                         return (rle->res);
                 }
         }
         return (resource_list_alloc(rl, dev, child, type, rid,
             start, end, count, flags));
 }
 
 I built a kernel with this change, booted it and the memory mapping 
 failure message was gone. All devices seemed to work but I was left 
 wondering if the return in the SYS_RES_MEMORY and SYS_RES_IOPORT case 
 also needs a call to bus_generic_activate_resource() and PCI_ENABLE_IO().