| Message ID | 20230912005655.368075-4-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix libata suspend/resume handling and code cleanup | expand |
Hi Damien, On Tue, 12 Sep 2023, Damien Le Moal wrote: > There is no direct device ancestry defined between an ata_device and > its scsi device which prevents the power management code from correctly > ordering suspend and resume operations. Create such ancestry with the > ata device as the parent to ensure that the scsi device (child) is > suspended before the ata device and that resume handles the ata device > before the scsi device. > > The parent-child (supplier-consumer) relationship is established between > the ata_port (parent) and the scsi device (child) with the function > device_add_link(). The parent used is not the ata_device as the PM > operations are defined per port and the status of all devices connected > through that port is controlled from the port operations. > > The device link is established with the new function > ata_scsi_dev_alloc(). This function is used to define the ->slave_alloc > callback of the scsi host template of most drivers. > > Fixes: a19a93e4c6a9 ("scsi: core: pm: Rely on the device driver core for async power management") > Cc: stable@vger.kernel.org > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > Reviewed-by: Hannes Reinecke <hare@suse.de> Thanks for your patch, which is now commit 99626085d036ec32 ("ata: libata-scsi: link ata port and scsi device") in libata/for-next. This patch causes /dev/sda to disappear on Renesas Salvator-XS with R-Car H3 ES2.0. Changes to dmesg before/after: sata_rcar ee300000.sata: ignoring dependency for device, assuming no driver scsi host0: sata_rcar -ata1: SATA max UDMA/133 irq 184 lpm-pol 0 +ata1: SATA max UDMA/133 irq 179 lpm-pol 0 ata1: link resume succeeded after 1 retries ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) ata1.00: ATA-7: Maxtor 6L160M0, BANC1G10, max UDMA/133 ata1.00: 320173056 sectors, multi 0: LBA48 NCQ (not used) ata1.00: configured for UDMA/133 scsi 0:0:0:0: Direct-Access ATA Maxtor 6L160M0 1G10 PQ: 0 ANSI: 5 -sd 0:0:0:0: [sda] 320173056 512-byte logical blocks: (164 GB/153 GiB) -sd 0:0:0:0: [sda] Write Protect is off -sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 -sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA -sd 0:0:0:0: [sda] Preferred minimum I/O size 512 bytes - sda: sda1 -sd 0:0:0:0: [sda] Attached SCSI disk Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Damien, On Wed, Sep 13, 2023 at 12:27 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, 12 Sep 2023, Damien Le Moal wrote: > > There is no direct device ancestry defined between an ata_device and > > its scsi device which prevents the power management code from correctly > > ordering suspend and resume operations. Create such ancestry with the > > ata device as the parent to ensure that the scsi device (child) is > > suspended before the ata device and that resume handles the ata device > > before the scsi device. > > > > The parent-child (supplier-consumer) relationship is established between > > the ata_port (parent) and the scsi device (child) with the function > > device_add_link(). The parent used is not the ata_device as the PM > > operations are defined per port and the status of all devices connected > > through that port is controlled from the port operations. > > > > The device link is established with the new function > > ata_scsi_dev_alloc(). This function is used to define the ->slave_alloc > > callback of the scsi host template of most drivers. > > > > Fixes: a19a93e4c6a9 ("scsi: core: pm: Rely on the device driver core for async power management") > > Cc: stable@vger.kernel.org > > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > > Reviewed-by: Hannes Reinecke <hare@suse.de> > > Thanks for your patch, which is now commit 99626085d036ec32 ("ata: > libata-scsi: link ata port and scsi device") in libata/for-next. > > This patch causes /dev/sda to disappear on Renesas Salvator-XS with > R-Car H3 ES2.0. Changes to dmesg before/after: > > sata_rcar ee300000.sata: ignoring dependency for device, assuming no driver > scsi host0: sata_rcar > -ata1: SATA max UDMA/133 irq 184 lpm-pol 0 > +ata1: SATA max UDMA/133 irq 179 lpm-pol 0 > ata1: link resume succeeded after 1 retries > ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) > ata1.00: ATA-7: Maxtor 6L160M0, BANC1G10, max UDMA/133 > ata1.00: 320173056 sectors, multi 0: LBA48 NCQ (not used) > ata1.00: configured for UDMA/133 > scsi 0:0:0:0: Direct-Access ATA Maxtor 6L160M0 1G10 PQ: 0 ANSI: 5 > -sd 0:0:0:0: [sda] 320173056 512-byte logical blocks: (164 GB/153 GiB) > -sd 0:0:0:0: [sda] Write Protect is off > -sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 > -sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA > -sd 0:0:0:0: [sda] Preferred minimum I/O size 512 bytes > - sda: sda1 > -sd 0:0:0:0: [sda] Attached SCSI disk I see the same issue on SH/Landisk, which has CompactFLASH: -ata1: PATA max PIO0 ioport cmd 0xc0023040 ctl 0xc002302c irq 26 +ata1: PATA max PIO0 ioport cmd 0xc0023040 ctl 0xc002302c irq 26 lpm-pol 0 ata1.00: CFA: TS8GCF133, 20171204, max UDMA/100 ata1.00: 15662304 sectors, multi 0: LBA48 ata1.00: configured for PIO scsi 0:0:0:0: Direct-Access ATA TS8GCF133 1204 PQ: 0 ANSI: 5 -sd 0:0:0:0: [sda] 15662304 512-byte logical blocks: (8.02 GB/7.47 GiB) -sd 0:0:0:0: [sda] Write Protect is off -sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 -sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA -sd 0:0:0:0: [sda] Preferred minimum I/O size 512 bytes - sda: sda1 sda2 sda3 -sd 0:0:0:0: [sda] Attached SCSI removable disk and m68k/ARAnyM: atari-falcon-ide atari-falcon-ide: Atari Falcon and Q40/Q60 PATA controller scsi host0: pata_falcon ata1: PATA max PIO4 cmd fff00000 ctl fff00038 data fff00000 no IRQ, using PIO polling ata1.00: ATA-2: Sarge m68k, , max PIO2 ata1.00: 2118816 sectors, multi 0: LBA ata1.00: configured for PIO scsi 0:0:0:0: Direct-Access ATA Sarge m68k n/a PQ: 0 ANSI: 5 -sd 0:0:0:0: [sda] 2118816 512-byte logical blocks: (1.08 GB/1.01 GiB) -sd 0:0:0:0: [sda] Write Protect is off -sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 -sd 0:0:0:0: [sda] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA -sd 0:0:0:0: [sda] Preferred minimum I/O size 512 bytes - sda: AHDI sda1 sda2 -sd 0:0:0:0: [sda] Attached SCSI disk Reverting 99626085d036ec32 fixes the issue. Gr{oetje,eeting}s, Geert
On 9/14/23 16:08, Geert Uytterhoeven wrote: > Hi Damien, > > On Wed, Sep 13, 2023 at 12:27 PM Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Tue, 12 Sep 2023, Damien Le Moal wrote: >>> There is no direct device ancestry defined between an ata_device and >>> its scsi device which prevents the power management code from correctly >>> ordering suspend and resume operations. Create such ancestry with the >>> ata device as the parent to ensure that the scsi device (child) is >>> suspended before the ata device and that resume handles the ata device >>> before the scsi device. >>> >>> The parent-child (supplier-consumer) relationship is established between >>> the ata_port (parent) and the scsi device (child) with the function >>> device_add_link(). The parent used is not the ata_device as the PM >>> operations are defined per port and the status of all devices connected >>> through that port is controlled from the port operations. >>> >>> The device link is established with the new function >>> ata_scsi_dev_alloc(). This function is used to define the ->slave_alloc >>> callback of the scsi host template of most drivers. >>> >>> Fixes: a19a93e4c6a9 ("scsi: core: pm: Rely on the device driver core for async power management") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>> Reviewed-by: Hannes Reinecke <hare@suse.de> >> >> Thanks for your patch, which is now commit 99626085d036ec32 ("ata: >> libata-scsi: link ata port and scsi device") in libata/for-next. >> >> This patch causes /dev/sda to disappear on Renesas Salvator-XS with >> R-Car H3 ES2.0. Changes to dmesg before/after: >> >> sata_rcar ee300000.sata: ignoring dependency for device, assuming no driver >> scsi host0: sata_rcar >> -ata1: SATA max UDMA/133 irq 184 lpm-pol 0 >> +ata1: SATA max UDMA/133 irq 179 lpm-pol 0 >> ata1: link resume succeeded after 1 retries >> ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) >> ata1.00: ATA-7: Maxtor 6L160M0, BANC1G10, max UDMA/133 >> ata1.00: 320173056 sectors, multi 0: LBA48 NCQ (not used) >> ata1.00: configured for UDMA/133 >> scsi 0:0:0:0: Direct-Access ATA Maxtor 6L160M0 1G10 PQ: 0 ANSI: 5 >> -sd 0:0:0:0: [sda] 320173056 512-byte logical blocks: (164 GB/153 GiB) >> -sd 0:0:0:0: [sda] Write Protect is off >> -sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 >> -sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA >> -sd 0:0:0:0: [sda] Preferred minimum I/O size 512 bytes >> - sda: sda1 >> -sd 0:0:0:0: [sda] Attached SCSI disk > > I see the same issue on SH/Landisk, which has CompactFLASH: > > -ata1: PATA max PIO0 ioport cmd 0xc0023040 ctl 0xc002302c irq 26 > +ata1: PATA max PIO0 ioport cmd 0xc0023040 ctl 0xc002302c irq 26 lpm-pol 0 > ata1.00: CFA: TS8GCF133, 20171204, max UDMA/100 > ata1.00: 15662304 sectors, multi 0: LBA48 > ata1.00: configured for PIO > scsi 0:0:0:0: Direct-Access ATA TS8GCF133 1204 > PQ: 0 ANSI: 5 > -sd 0:0:0:0: [sda] 15662304 512-byte logical blocks: (8.02 GB/7.47 GiB) > -sd 0:0:0:0: [sda] Write Protect is off > -sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 > -sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, > doesn't support DPO or FUA > -sd 0:0:0:0: [sda] Preferred minimum I/O size 512 bytes > - sda: sda1 sda2 sda3 > -sd 0:0:0:0: [sda] Attached SCSI removable disk > > and m68k/ARAnyM: > > atari-falcon-ide atari-falcon-ide: Atari Falcon and Q40/Q60 PATA controller > scsi host0: pata_falcon > ata1: PATA max PIO4 cmd fff00000 ctl fff00038 data fff00000 no > IRQ, using PIO polling > ata1.00: ATA-2: Sarge m68k, , max PIO2 > ata1.00: 2118816 sectors, multi 0: LBA > ata1.00: configured for PIO > scsi 0:0:0:0: Direct-Access ATA Sarge m68k n/a > PQ: 0 ANSI: 5 > -sd 0:0:0:0: [sda] 2118816 512-byte logical blocks: (1.08 GB/1.01 GiB) > -sd 0:0:0:0: [sda] Write Protect is off > -sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 > -sd 0:0:0:0: [sda] Write cache: disabled, read cache: enabled, > doesn't support DPO or FUA > -sd 0:0:0:0: [sda] Preferred minimum I/O size 512 bytes > - sda: AHDI sda1 sda2 > -sd 0:0:0:0: [sda] Attached SCSI disk > > Reverting 99626085d036ec32 fixes the issue. Yes. I can confirm this. I can recreate the issue, because I have a major screw-up with that patch: the device_link_add() done in the new ->slave_alloc operation for the scsi_host_template for the driver was in fact NOT set for AHCI. So all my testing confirming that suspend/resume is OK was done *without* that device link being created... That is embarrassing :) The sata_rcar driver does get that slave_alloc method set through ATA_BASE_SHT() -> ATA_SUBBASE_SHT(), and you get the problem. If I fix AHCI_SHT() macro to set slave_alloc, I do get the issue as well: no scsi disk device is created after the port scan. No clue why. And I also now have no clue how suddenly the suspend/resume operations get magically ordered correctly... I could recreate various issues before the patches in for-6.7. So going back to the beginning to sort things out. I probably "inadvertently" fixed the issues with another change that has implications I am not seeing. Will get back to you ASAP. > > Gr{oetje,eeting}s, > > Geert >
On 9/14/23 16:08, Geert Uytterhoeven wrote: > Hi Damien, > > On Wed, Sep 13, 2023 at 12:27 PM Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Tue, 12 Sep 2023, Damien Le Moal wrote: >>> There is no direct device ancestry defined between an ata_device and >>> its scsi device which prevents the power management code from correctly >>> ordering suspend and resume operations. Create such ancestry with the >>> ata device as the parent to ensure that the scsi device (child) is >>> suspended before the ata device and that resume handles the ata device >>> before the scsi device. >>> >>> The parent-child (supplier-consumer) relationship is established between >>> the ata_port (parent) and the scsi device (child) with the function >>> device_add_link(). The parent used is not the ata_device as the PM >>> operations are defined per port and the status of all devices connected >>> through that port is controlled from the port operations. >>> >>> The device link is established with the new function >>> ata_scsi_dev_alloc(). This function is used to define the ->slave_alloc >>> callback of the scsi host template of most drivers. >>> >>> Fixes: a19a93e4c6a9 ("scsi: core: pm: Rely on the device driver core for async power management") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>> Reviewed-by: Hannes Reinecke <hare@suse.de> >> >> Thanks for your patch, which is now commit 99626085d036ec32 ("ata: >> libata-scsi: link ata port and scsi device") in libata/for-next. >> >> This patch causes /dev/sda to disappear on Renesas Salvator-XS with >> R-Car H3 ES2.0. Changes to dmesg before/after: >> >> sata_rcar ee300000.sata: ignoring dependency for device, assuming no driver >> scsi host0: sata_rcar >> -ata1: SATA max UDMA/133 irq 184 lpm-pol 0 >> +ata1: SATA max UDMA/133 irq 179 lpm-pol 0 >> ata1: link resume succeeded after 1 retries >> ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) >> ata1.00: ATA-7: Maxtor 6L160M0, BANC1G10, max UDMA/133 >> ata1.00: 320173056 sectors, multi 0: LBA48 NCQ (not used) >> ata1.00: configured for UDMA/133 >> scsi 0:0:0:0: Direct-Access ATA Maxtor 6L160M0 1G10 PQ: 0 ANSI: 5 >> -sd 0:0:0:0: [sda] 320173056 512-byte logical blocks: (164 GB/153 GiB) >> -sd 0:0:0:0: [sda] Write Protect is off >> -sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 >> -sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA >> -sd 0:0:0:0: [sda] Preferred minimum I/O size 512 bytes >> - sda: sda1 >> -sd 0:0:0:0: [sda] Attached SCSI disk > > I see the same issue on SH/Landisk, which has CompactFLASH: > > -ata1: PATA max PIO0 ioport cmd 0xc0023040 ctl 0xc002302c irq 26 > +ata1: PATA max PIO0 ioport cmd 0xc0023040 ctl 0xc002302c irq 26 lpm-pol 0 > ata1.00: CFA: TS8GCF133, 20171204, max UDMA/100 > ata1.00: 15662304 sectors, multi 0: LBA48 > ata1.00: configured for PIO > scsi 0:0:0:0: Direct-Access ATA TS8GCF133 1204 > PQ: 0 ANSI: 5 > -sd 0:0:0:0: [sda] 15662304 512-byte logical blocks: (8.02 GB/7.47 GiB) > -sd 0:0:0:0: [sda] Write Protect is off > -sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 > -sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, > doesn't support DPO or FUA > -sd 0:0:0:0: [sda] Preferred minimum I/O size 512 bytes > - sda: sda1 sda2 sda3 > -sd 0:0:0:0: [sda] Attached SCSI removable disk > > and m68k/ARAnyM: > > atari-falcon-ide atari-falcon-ide: Atari Falcon and Q40/Q60 PATA controller > scsi host0: pata_falcon > ata1: PATA max PIO4 cmd fff00000 ctl fff00038 data fff00000 no > IRQ, using PIO polling > ata1.00: ATA-2: Sarge m68k, , max PIO2 > ata1.00: 2118816 sectors, multi 0: LBA > ata1.00: configured for PIO > scsi 0:0:0:0: Direct-Access ATA Sarge m68k n/a > PQ: 0 ANSI: 5 > -sd 0:0:0:0: [sda] 2118816 512-byte logical blocks: (1.08 GB/1.01 GiB) > -sd 0:0:0:0: [sda] Write Protect is off > -sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 > -sd 0:0:0:0: [sda] Write cache: disabled, read cache: enabled, > doesn't support DPO or FUA > -sd 0:0:0:0: [sda] Preferred minimum I/O size 512 bytes > - sda: AHDI sda1 sda2 > -sd 0:0:0:0: [sda] Attached SCSI disk > > Reverting 99626085d036ec32 fixes the issue. Without reverting, can you try this incremental update ? diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h index 4bae95b06ae3..72085756f4ba 100644 --- a/drivers/ata/ahci.h +++ b/drivers/ata/ahci.h @@ -398,6 +398,7 @@ extern const struct attribute_group *ahci_sdev_groups[]; .sdev_groups = ahci_sdev_groups, \ .change_queue_depth = ata_scsi_change_queue_depth, \ .tag_alloc_policy = BLK_TAG_ALLOC_RR, \ + .slave_alloc = ata_scsi_slave_alloc, \ .slave_configure = ata_scsi_slave_config extern struct ata_port_operations ahci_ops; diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 7aa70af1fc07..5a0513452150 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -1093,6 +1093,7 @@ int ata_scsi_dev_alloc(struct scsi_device *sdev, struct ata_port *ap) * consumer (child) and the ata port the supplier (parent). */ link = device_link_add(&sdev->sdev_gendev, &ap->tdev, + DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE); if (!link) { ata_port_err(ap, "Failed to create link to scsi device %s\n", @@ -1164,6 +1165,8 @@ void ata_scsi_slave_destroy(struct scsi_device *sdev) unsigned long flags; struct ata_device *dev; + device_link_remove(&sdev->sdev_gendev, &ap->tdev); + spin_lock_irqsave(ap->lock, flags); dev = __ata_scsi_find_dev(ap, sdev); if (dev && dev->sdev) { This solves the issue for me. If you confirm it works for you, I will squash this into 99626085d036ec32. Thanks !
Hi Damien, On Thu, Sep 14, 2023 at 3:18 PM Damien Le Moal <dlemoal@kernel.org> wrote: > On 9/14/23 16:08, Geert Uytterhoeven wrote: > > On Wed, Sep 13, 2023 at 12:27 PM Geert Uytterhoeven > > <geert@linux-m68k.org> wrote: > >> On Tue, 12 Sep 2023, Damien Le Moal wrote: > >>> There is no direct device ancestry defined between an ata_device and > >>> its scsi device which prevents the power management code from correctly > >>> ordering suspend and resume operations. Create such ancestry with the > >>> ata device as the parent to ensure that the scsi device (child) is > >>> suspended before the ata device and that resume handles the ata device > >>> before the scsi device. > >>> > >>> The parent-child (supplier-consumer) relationship is established between > >>> the ata_port (parent) and the scsi device (child) with the function > >>> device_add_link(). The parent used is not the ata_device as the PM > >>> operations are defined per port and the status of all devices connected > >>> through that port is controlled from the port operations. > >>> > >>> The device link is established with the new function > >>> ata_scsi_dev_alloc(). This function is used to define the ->slave_alloc > >>> callback of the scsi host template of most drivers. > >>> > >>> Fixes: a19a93e4c6a9 ("scsi: core: pm: Rely on the device driver core for async power management") > >>> Cc: stable@vger.kernel.org > >>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > >>> Reviewed-by: Hannes Reinecke <hare@suse.de> > >> > >> Thanks for your patch, which is now commit 99626085d036ec32 ("ata: > >> libata-scsi: link ata port and scsi device") in libata/for-next. > >> > >> This patch causes /dev/sda to disappear on Renesas Salvator-XS with > >> R-Car H3 ES2.0. Changes to dmesg before/after: > >> > >> sata_rcar ee300000.sata: ignoring dependency for device, assuming no driver > >> scsi host0: sata_rcar > >> -ata1: SATA max UDMA/133 irq 184 lpm-pol 0 > >> +ata1: SATA max UDMA/133 irq 179 lpm-pol 0 > >> ata1: link resume succeeded after 1 retries > >> ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) > >> ata1.00: ATA-7: Maxtor 6L160M0, BANC1G10, max UDMA/133 > >> ata1.00: 320173056 sectors, multi 0: LBA48 NCQ (not used) > >> ata1.00: configured for UDMA/133 > >> scsi 0:0:0:0: Direct-Access ATA Maxtor 6L160M0 1G10 PQ: 0 ANSI: 5 > >> -sd 0:0:0:0: [sda] 320173056 512-byte logical blocks: (164 GB/153 GiB) > >> -sd 0:0:0:0: [sda] Write Protect is off > >> -sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 > >> -sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA > >> -sd 0:0:0:0: [sda] Preferred minimum I/O size 512 bytes > >> - sda: sda1 > >> -sd 0:0:0:0: [sda] Attached SCSI disk > > > > I see the same issue on SH/Landisk, which has CompactFLASH: > > and m68k/ARAnyM: > > Reverting 99626085d036ec32 fixes the issue. > > Without reverting, can you try this incremental update ? > > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h > index 4bae95b06ae3..72085756f4ba 100644 > --- a/drivers/ata/ahci.h > +++ b/drivers/ata/ahci.h > @@ -398,6 +398,7 @@ extern const struct attribute_group *ahci_sdev_groups[]; > .sdev_groups = ahci_sdev_groups, \ > .change_queue_depth = ata_scsi_change_queue_depth, \ > .tag_alloc_policy = BLK_TAG_ALLOC_RR, \ > + .slave_alloc = ata_scsi_slave_alloc, \ > .slave_configure = ata_scsi_slave_config > > extern struct ata_port_operations ahci_ops; > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 7aa70af1fc07..5a0513452150 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -1093,6 +1093,7 @@ int ata_scsi_dev_alloc(struct scsi_device *sdev, struct ata_port *ap) > * consumer (child) and the ata port the supplier (parent). > */ > link = device_link_add(&sdev->sdev_gendev, &ap->tdev, > + DL_FLAG_STATELESS | > DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE); > if (!link) { > ata_port_err(ap, "Failed to create link to scsi device %s\n", > @@ -1164,6 +1165,8 @@ void ata_scsi_slave_destroy(struct scsi_device *sdev) > unsigned long flags; > struct ata_device *dev; > > + device_link_remove(&sdev->sdev_gendev, &ap->tdev); > + > spin_lock_irqsave(ap->lock, flags); > dev = __ata_scsi_find_dev(ap, sdev); > if (dev && dev->sdev) { > > This solves the issue for me. If you confirm it works for you, I will squash > this into 99626085d036ec32. Thank you, /dev/sda is back into business on R-Car SATA, Landisk, and ARAnyM. Gr{oetje,eeting}s, Geert
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index d3f28b82c97b..f63cf6e7332e 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -1089,6 +1089,45 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev) return 0; } +int ata_scsi_dev_alloc(struct scsi_device *sdev, struct ata_port *ap) +{ + struct device_link *link; + + ata_scsi_sdev_config(sdev); + + /* + * Create a link from the ata_port device to the scsi device to ensure + * that PM does suspend/resume in the correct order: the scsi device is + * consumer (child) and the ata port the supplier (parent). + */ + link = device_link_add(&sdev->sdev_gendev, &ap->tdev, + DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE); + if (!link) { + ata_port_err(ap, "Failed to create link to scsi device %s\n", + dev_name(&sdev->sdev_gendev)); + return -ENODEV; + } + + return 0; +} + +/** + * ata_scsi_slave_alloc - Early setup of SCSI device + * @sdev: SCSI device to examine + * + * This is called from scsi_alloc_sdev() when the scsi device + * associated with an ATA device is scanned on a port. + * + * LOCKING: + * Defined by SCSI layer. We don't really care. + */ + +int ata_scsi_slave_alloc(struct scsi_device *sdev) +{ + return ata_scsi_dev_alloc(sdev, ata_shost_to_port(sdev->host)); +} +EXPORT_SYMBOL_GPL(ata_scsi_slave_alloc); + /** * ata_scsi_slave_config - Set SCSI device attributes * @sdev: SCSI device to examine @@ -1105,14 +1144,11 @@ int ata_scsi_slave_config(struct scsi_device *sdev) { struct ata_port *ap = ata_shost_to_port(sdev->host); struct ata_device *dev = __ata_scsi_find_dev(ap, sdev); - int rc = 0; - - ata_scsi_sdev_config(sdev); if (dev) - rc = ata_scsi_dev_config(sdev, dev); + return ata_scsi_dev_config(sdev, dev); - return rc; + return 0; } EXPORT_SYMBOL_GPL(ata_scsi_slave_config); diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h index 6e7d352803bd..079981e7156a 100644 --- a/drivers/ata/libata.h +++ b/drivers/ata/libata.h @@ -111,6 +111,7 @@ extern struct ata_device *ata_scsi_find_dev(struct ata_port *ap, extern int ata_scsi_add_hosts(struct ata_host *host, const struct scsi_host_template *sht); extern void ata_scsi_scan_host(struct ata_port *ap, int sync); +extern int ata_scsi_dev_alloc(struct scsi_device *sdev, struct ata_port *ap); extern int ata_scsi_offline_dev(struct ata_device *dev); extern bool ata_scsi_sense_is_valid(u8 sk, u8 asc, u8 ascq); extern void ata_scsi_set_sense(struct ata_device *dev, diff --git a/drivers/ata/pata_macio.c b/drivers/ata/pata_macio.c index 17f6ccee53c7..32968b4cf8e4 100644 --- a/drivers/ata/pata_macio.c +++ b/drivers/ata/pata_macio.c @@ -918,6 +918,7 @@ static const struct scsi_host_template pata_macio_sht = { * use 64K minus 256 */ .max_segment_size = MAX_DBDMA_SEG, + .slave_alloc = ata_scsi_slave_alloc, .slave_configure = pata_macio_slave_config, .sdev_groups = ata_common_sdev_groups, .can_queue = ATA_DEF_QUEUE, diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c index d105db5c7d81..353ac7b2f14a 100644 --- a/drivers/ata/sata_mv.c +++ b/drivers/ata/sata_mv.c @@ -673,6 +673,7 @@ static const struct scsi_host_template mv6_sht = { .sdev_groups = ata_ncq_sdev_groups, .change_queue_depth = ata_scsi_change_queue_depth, .tag_alloc_policy = BLK_TAG_ALLOC_RR, + .slave_alloc = ata_scsi_slave_alloc, .slave_configure = ata_scsi_slave_config }; diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c index 0a0cee755bde..5428dc2ec5e3 100644 --- a/drivers/ata/sata_nv.c +++ b/drivers/ata/sata_nv.c @@ -380,6 +380,7 @@ static const struct scsi_host_template nv_adma_sht = { .can_queue = NV_ADMA_MAX_CPBS, .sg_tablesize = NV_ADMA_SGTBL_TOTAL_LEN, .dma_boundary = NV_ADMA_DMA_BOUNDARY, + .slave_alloc = ata_scsi_slave_alloc, .slave_configure = nv_adma_slave_config, .sdev_groups = ata_ncq_sdev_groups, .change_queue_depth = ata_scsi_change_queue_depth, @@ -391,6 +392,7 @@ static const struct scsi_host_template nv_swncq_sht = { .can_queue = ATA_MAX_QUEUE - 1, .sg_tablesize = LIBATA_MAX_PRD, .dma_boundary = ATA_DMA_BOUNDARY, + .slave_alloc = ata_scsi_slave_alloc, .slave_configure = nv_swncq_slave_config, .sdev_groups = ata_ncq_sdev_groups, .change_queue_depth = ata_scsi_change_queue_depth, diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c index 142e70bfc498..e0b1b3625031 100644 --- a/drivers/ata/sata_sil24.c +++ b/drivers/ata/sata_sil24.c @@ -381,6 +381,7 @@ static const struct scsi_host_template sil24_sht = { .tag_alloc_policy = BLK_TAG_ALLOC_FIFO, .sdev_groups = ata_ncq_sdev_groups, .change_queue_depth = ata_scsi_change_queue_depth, + .slave_alloc = ata_scsi_slave_alloc, .slave_configure = ata_scsi_slave_config }; diff --git a/include/linux/libata.h b/include/linux/libata.h index 52d58b13e5ee..c8cfea386c16 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -1144,6 +1144,7 @@ extern int ata_std_bios_param(struct scsi_device *sdev, struct block_device *bdev, sector_t capacity, int geom[]); extern void ata_scsi_unlock_native_capacity(struct scsi_device *sdev); +extern int ata_scsi_slave_alloc(struct scsi_device *sdev); extern int ata_scsi_slave_config(struct scsi_device *sdev); extern void ata_scsi_slave_destroy(struct scsi_device *sdev); extern int ata_scsi_change_queue_depth(struct scsi_device *sdev, @@ -1401,12 +1402,14 @@ extern const struct attribute_group *ata_common_sdev_groups[]; __ATA_BASE_SHT(drv_name), \ .can_queue = ATA_DEF_QUEUE, \ .tag_alloc_policy = BLK_TAG_ALLOC_RR, \ + .slave_alloc = ata_scsi_slave_alloc, \ .slave_configure = ata_scsi_slave_config #define ATA_SUBBASE_SHT_QD(drv_name, drv_qd) \ __ATA_BASE_SHT(drv_name), \ .can_queue = drv_qd, \ .tag_alloc_policy = BLK_TAG_ALLOC_RR, \ + .slave_alloc = ata_scsi_slave_alloc, \ .slave_configure = ata_scsi_slave_config #define ATA_BASE_SHT(drv_name) \
There is no direct device ancestry defined between an ata_device and its scsi device which prevents the power management code from correctly ordering suspend and resume operations. Create such ancestry with the ata device as the parent to ensure that the scsi device (child) is suspended before the ata device and that resume handles the ata device before the scsi device. The parent-child (supplier-consumer) relationship is established between the ata_port (parent) and the scsi device (child) with the function device_add_link(). The parent used is not the ata_device as the PM operations are defined per port and the status of all devices connected through that port is controlled from the port operations. The device link is established with the new function ata_scsi_dev_alloc(). This function is used to define the ->slave_alloc callback of the scsi host template of most drivers. Fixes: a19a93e4c6a9 ("scsi: core: pm: Rely on the device driver core for async power management") Cc: stable@vger.kernel.org Signed-off-by: Damien Le Moal <dlemoal@kernel.org> Reviewed-by: Hannes Reinecke <hare@suse.de> --- drivers/ata/libata-scsi.c | 46 ++++++++++++++++++++++++++++++++++----- drivers/ata/libata.h | 1 + drivers/ata/pata_macio.c | 1 + drivers/ata/sata_mv.c | 1 + drivers/ata/sata_nv.c | 2 ++ drivers/ata/sata_sil24.c | 1 + include/linux/libata.h | 3 +++ 7 files changed, 50 insertions(+), 5 deletions(-)