Re: [PATCH] libata: fix combined mode (was Re: Happy New Year (and v2.6.20-rc3 released))

["Alessandro Suardi" <alessandro.suardi@gmail.com>]

On 1/2/07, Alan <alan@lxorguk.ukuu.org.uk> wrote:
> This is a slight variant on the patch I posted December 16th to fix
> libata combined mode handling. The only real change is that we now
> correctly also reserve BAR1,2,4. That is basically a neatness issue.
>
> Jeff was unhappy about two things
>
> 1. That it didn't work in the case of one channel native one channel
> legacy.
>
> This is a silly complaint because the SFF layer in libata doesn't handle
> this case yet anyway.
>
> 2. The case where combined mode is in use and IDE=n.
>
> In this case the libata quirk code reserves the resources in question
> correctly already.
>
> Once the combined mode stuff is redone properly (2.6.21) then the entire
> mess turns into a single pci_request_regions() for all cases and all the
> ugly resource hackery goes away.
>
> I'm sending this now rather than after running full test suites so that
> it can get the maximal testing in a short time. I'll be running tests on
> this after lunch.
>
> Signed-off-by: Alan Cox <alan@redhat.com>
>
> --- linux.vanilla-2.6.20-rc3/drivers/ata/libata-sff.c   2007-01-01 21:43:27.000000000 +0000
> +++ linux-2.6.20-rc3/drivers/ata/libata-sff.c   2007-01-02 11:15:53.000000000 +0000
> @@ -1027,13 +1027,15 @@
>  #endif
>         }
>
> -       rc = pci_request_regions(pdev, DRV_NAME);
> -       if (rc) {
> -               disable_dev_on_err = 0;
> -               goto err_out;
> -       }
> -
> -       if (legacy_mode) {
> +       if (!legacy_mode) {
> +               rc = pci_request_regions(pdev, DRV_NAME);
> +               if (rc) {
> +                       disable_dev_on_err = 0;
> +                       goto err_out;
> +               }
> +       } else {
> +               /* Deal with combined mode hack. This side of the logic all
> +                  goes away once the combined mode hack is killed in 2.6.21 */
>                 if (!request_region(ATA_PRIMARY_CMD, 8, "libata")) {
>                         struct resource *conflict, res;
>                         res.start = ATA_PRIMARY_CMD;
> @@ -1071,6 +1073,13 @@
>                         }
>                 } else
>                         legacy_mode |= ATA_PORT_SECONDARY;
> +
> +               if (legacy_mode & ATA_PORT_PRIMARY)
> +                       pci_request_region(pdev, 1, DRV_NAME);
> +               if (legacy_mode & ATA_PORT_SECONDARY)
> +                       pci_request_region(pdev, 3, DRV_NAME);
> +               /* If there is a DMA resource, allocate it */
> +               pci_request_region(pdev, 4, DRV_NAME);
>         }
>
>         /* we have legacy mode, but all ports are unavailable */
> @@ -1114,11 +1123,20 @@
>  err_out_ent:
>         kfree(probe_ent);
>  err_out_regions:
> -       if (legacy_mode & ATA_PORT_PRIMARY)
> -               release_region(ATA_PRIMARY_CMD, 8);
> -       if (legacy_mode & ATA_PORT_SECONDARY)
> -               release_region(ATA_SECONDARY_CMD, 8);
> -       pci_release_regions(pdev);
> +       /* All this conditional stuff is needed for the combined mode hack
> +          until 2.6.21 when it can go */
> +       if (legacy_mode) {
> +               pci_release_region(pdev, 4);
> +               if (legacy_mode & ATA_PORT_PRIMARY) {
> +                       release_region(ATA_PRIMARY_CMD, 8);
> +                       pci_release_region(pdev, 1);
> +               }
> +               if (legacy_mode & ATA_PORT_SECONDARY) {
> +                       release_region(ATA_SECONDARY_CMD, 8);
> +                       pci_release_region(pdev, 3);
> +               }
> +       } else
> +               pci_release_regions(pdev);
>  err_out:
>         if (disable_dev_on_err)
>                 pci_disable_device(pdev);
>

Appears to work just fine here (compiles, boots and I'm
 typing this email :).  The build warnings below seem new
 to me - but I guess they're harmless...

  CC      drivers/ata/libata-sff.o
drivers/ata/libata-sff.c: In function 'ata_pci_init_one':
drivers/ata/libata-sff.c:1078: warning: ignoring return value of
'pci_request_region', declared with attribute warn_unused_result
drivers/ata/libata-sff.c:1080: warning: ignoring return value of
'pci_request_region', declared with attribute warn_unused_result
drivers/ata/libata-sff.c:1082: warning: ignoring return value of
'pci_request_region', declared with attribute warn_unused_result

Thanks, ciao,

--alessandro

 "but I thought that I should let you know
  the things that I don't always show
  might not be worth the time it took"

     (Steve Wynn, 'If My Life Was An Open Book')