| Message ID | 20230911040217.253905-4-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix libata suspend/resume handling and code cleanup | expand |
On 9/11/23 06:02, 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> > --- > 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(-) > > 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) \ I do understand the rationale here, as the relationship between ata port and scsi devices are blurred. Question is: blurred by what? Is it the libata/SAS duality where SCSI devices will have a different ancestry for libata and libsas? If so, why don't we need the 'link' mechanism for SAS? Or is it something else? Cheers, Hannes
On 9/11/23 15:41, Hannes Reinecke wrote: > On 9/11/23 06:02, 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> >> --- >> 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(-) >> >> 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) \ > I do understand the rationale here, as the relationship between ata port and > scsi devices are blurred. Question is: blurred by what? > Is it the libata/SAS duality where SCSI devices will have a different > ancestry for libata and libsas? > If so, why don't we need the 'link' mechanism for SAS? > Or is it something else? On scsi, ancestry from Scsi_Host is clear: host->target->device->disk. For ATA, it is also clear: port->link->device The relationship is that ata port has its own Scsi_Host, but no "family" relationship here. They are just "friends", so scsi disk and ata_port have no direct ancestry. Adding the link creates that. libsas does not need the link because it does its own PM management of the ports, not relying on PM to order things. > > Cheers, > > Hannes
On 9/11/23 08:48, Damien Le Moal wrote: > On 9/11/23 15:41, Hannes Reinecke wrote: >> On 9/11/23 06:02, 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> >>> --- >>> 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(-) >>> >>> 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) \ >> I do understand the rationale here, as the relationship between ata port and >> scsi devices are blurred. Question is: blurred by what? >> Is it the libata/SAS duality where SCSI devices will have a different >> ancestry for libata and libsas? >> If so, why don't we need the 'link' mechanism for SAS? >> Or is it something else? > > On scsi, ancestry from Scsi_Host is clear: host->target->device->disk. > For ATA, it is also clear: port->link->device > > The relationship is that ata port has its own Scsi_Host, but no "family" > relationship here. They are just "friends", so scsi disk and ata_port have no > direct ancestry. Adding the link creates that. > > libsas does not need the link because it does its own PM management of the > ports, not relying on PM to order things. > Okay. Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 11/09/2023 07:48, Damien Le Moal wrote: >>> #define ATA_BASE_SHT(drv_name) \ >> I do understand the rationale here, as the relationship between ata port and >> scsi devices are blurred. Question is: blurred by what? >> Is it the libata/SAS duality where SCSI devices will have a different >> ancestry for libata and libsas? >> If so, why don't we need the 'link' mechanism for SAS? >> Or is it something else? > On scsi, ancestry from Scsi_Host is clear: host->target->device->disk. > For ATA, it is also clear: port->link->device > > The relationship is that ata port has its own Scsi_Host, but no "family" > relationship here. They are just "friends", so scsi disk and ata_port have no > direct ancestry. Adding the link creates that. > > libsas does not need the link because it does its own PM management of the > ports, not relying on PM to order things. > For hisi_sas v3, RPM is supported and a link was required to be added in that driver between the sdev and those host controller device - see 16fd4a7c59. And that is for a similar reason here. I see that we already have an ancestry for libata between ata_dev -> ata_link -> ata_port -> ata_host dev, so it seems reasonable to add ata_dev <-> sdev dependency here. I think that this should really be added for all libsas drivers which support RPM - pm8001 is missing, IIRC. Thanks, John
On 9/11/23 19:38, John Garry wrote: > On 11/09/2023 07:48, Damien Le Moal wrote: >>>> #define ATA_BASE_SHT(drv_name) \ >>> I do understand the rationale here, as the relationship between ata port and >>> scsi devices are blurred. Question is: blurred by what? >>> Is it the libata/SAS duality where SCSI devices will have a different >>> ancestry for libata and libsas? >>> If so, why don't we need the 'link' mechanism for SAS? >>> Or is it something else? >> On scsi, ancestry from Scsi_Host is clear: host->target->device->disk. >> For ATA, it is also clear: port->link->device >> >> The relationship is that ata port has its own Scsi_Host, but no "family" >> relationship here. They are just "friends", so scsi disk and ata_port have no >> direct ancestry. Adding the link creates that. >> >> libsas does not need the link because it does its own PM management of the >> ports, not relying on PM to order things. >> > > For hisi_sas v3, RPM is supported and a link was required to be added in > that driver between the sdev and those host controller device - see > 16fd4a7c59. And that is for a similar reason here. I see that we already > have an ancestry for libata between ata_dev -> ata_link -> ata_port -> > ata_host dev, so it seems reasonable to add ata_dev <-> sdev dependency > here. libata creates a link between ata_port and sdev. This is not very natural, but as I said in the cover letter, the power management model is weird as everything is per port, even if a port has multiple devices. Nevertheless, I tried to apply the same for libsas but failed: if I add the link creation in the slave_alloc method, I fail to get the scsi disks to show up (no /dev/sdX device files). Not sure why. That was with my pm8001 adapter, which is the only libsas one I have. Side note: having an ata_debug (ata equivalent of scsi_debug) driver that could act as a pure libata driver or as a libsas adapter would really be awesome for this kind of tests :) > I think that this should really be added for all libsas drivers which > support RPM - pm8001 is missing, IIRC. Will try again tomorrow looking at hisi driver for inspiration. The other thing I need to better understand is how the suspend & resume operations of libsas get triggered. For suspend, it is easy-ish, but for resume, it seems to be tightly coupled with discovery, so the the adapter resume (scsi host class resume ?). If that is the case, then that is done *before* the scsi_dev resume because of the existing device link dependencies. So I am not yet entirely convinced that adding a link between ata_port and sdev will serve any purpose now that the asynchronous libata resume is fixed and synchronized with the scsi disk resume... Will dig further. That said, it may be good to move libsas suspend/resume fixes to another patch series. There is an outstanding regression/problem for pure libata devices that this series fixes. So would like to get the fixes patches in Linus tree ASAP. Cheers.
On 11/09/2023 12:48, Damien Le Moal wrote: >> For hisi_sas v3, RPM is supported and a link was required to be added in >> that driver between the sdev and those host controller device - see >> 16fd4a7c59. And that is for a similar reason here. I see that we already >> have an ancestry for libata between ata_dev -> ata_link -> ata_port -> >> ata_host dev, so it seems reasonable to add ata_dev <-> sdev dependency >> here. > libata creates a link between ata_port and sdev. This is not very natural, but > as I said in the cover letter, the power management model is weird as everything > is per port, even if a port has multiple devices. Nevertheless, I tried to apply > the same for libsas but failed: if I add the link creation in the slave_alloc > method, I fail to get the scsi disks to show up (no /dev/sdX device files). Not > sure why. That was with my pm8001 adapter, which is the only libsas one I have. > > Side note: having an ata_debug (ata equivalent of scsi_debug) driver that could > act as a pure libata driver or as a libsas adapter would really be awesome for > this kind of tests
On 9/12/23 00:15, John Garry wrote: > On 11/09/2023 12:48, Damien Le Moal wrote: >>> For hisi_sas v3, RPM is supported and a link was required to be added in >>> that driver between the sdev and those host controller device - see >>> 16fd4a7c59. And that is for a similar reason here. I see that we already >>> have an ancestry for libata between ata_dev -> ata_link -> ata_port -> >>> ata_host dev, so it seems reasonable to add ata_dev <-> sdev dependency >>> here. >> libata creates a link between ata_port and sdev. This is not very natural, but >> as I said in the cover letter, the power management model is weird as everything >> is per port, even if a port has multiple devices. Nevertheless, I tried to apply >> the same for libsas but failed: if I add the link creation in the slave_alloc >> method, I fail to get the scsi disks to show up (no /dev/sdX device files). Not >> sure why. That was with my pm8001 adapter, which is the only libsas one I have. >> >> Side note: having an ata_debug (ata equivalent of scsi_debug) driver that could >> act as a pure libata driver or as a libsas adapter would really be awesome for >> this kind of tests
On 12/09/2023 07:13, Damien Le Moal wrote: >> hmm... maybe scsi_debug could be modified to support ATA devices (with >> libata). >> >> Regardless, I'm happy to check the code change which you attempted if >> shared. > I had a closer look at what the hisi_sas driver is doing. The link it creates > with device_add_link() is between the scsi device gendev and the hisi_hba->dev, > that is, the HBA device. > So nothing to do with libata port of device. Correct, it's just for the sdev -> host dev link > > Trying to mimic what libata is now doing, I tried this: > > diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c > index 12e2653846e3..91d76258e6ea 100644 > --- a/drivers/scsi/libsas/sas_ata.c > +++ b/drivers/scsi/libsas/sas_ata.c > @@ -624,6 +624,26 @@ int sas_ata_init(struct domain_device *found_dev) > return rc; > } > > +int sas_ata_slave_configure(struct scsi_device *sdev) > +{ > + struct domain_device *dev = sdev_to_domain_dev(sdev); > + struct ata_port *ap = dev->sata_dev.ap; > + struct device *sdev_dev = &sdev->sdev_gendev; > + struct device_link *link; > + > + ata_sas_slave_configure(sdev, ap); > + > + link = device_link_add(sdev_dev, &ap->tdev, > + DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE); I am not sure what is the point of this. For libsas/pm8001 driver, there is no ata_port -> .. -> host dev link, right? If so, it just seems that you are are adding a dependency to a device (&ap->tdev) which has no dependency to a real device itself. > + if (!link && pm_runtime_enabled(sdev_dev)) { > + dev_err(sdev_dev, > + "add device link failed, disabling runtime PM for the > host\n"); > + pm_runtime_disable(sdev_dev); > + } > + > + return 0; > +} > + > void sas_ata_task_abort(struct sas_task *task) > { > struct ata_queued_cmd *qc = task->uldd_task; > diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h > index a6dc7dc07fce..82c6d8e1e8c7 100644 > --- a/drivers/scsi/libsas/sas_internal.h > +++ b/drivers/scsi/libsas/sas_internal.h > @@ -58,6 +58,8 @@ void sas_enable_revalidation(struct sas_ha_struct *ha); > void sas_queue_deferred_work(struct sas_ha_struct *ha); > void __sas_drain_work(struct sas_ha_struct *ha); > > +int sas_ata_slave_configure(struct scsi_device *sdev); > + > void sas_deform_port(struct asd_sas_phy *phy, int gone); > > void sas_porte_bytes_dmaed(struct work_struct *work); > diff --git a/drivers/scsi/libsas/sas_scsi_host.c > b/drivers/scsi/libsas/sas_scsi_host.c > index 9047cfcd1072..b6c419aa563e 100644 > --- a/drivers/scsi/libsas/sas_scsi_host.c > +++ b/drivers/scsi/libsas/sas_scsi_host.c > @@ -810,10 +810,8 @@ int sas_slave_configure(struct scsi_device *scsi_dev) > > BUG_ON(dev->rphy->identify.device_type != SAS_END_DEVICE); > > - if (dev_is_sata(dev)) { > - ata_sas_slave_configure(scsi_dev, dev->sata_dev.ap); > - return 0; > - } > + if (dev_is_sata(dev)) > + return sas_ata_slave_configure(scsi_dev); > > sas_read_port_mode_page(scsi_dev); > > This does not change the drive discovery, all drives connected to the HBA are > found and identified: > > [ 52.974154] scsi host12: pm80xx > [ 54.445986] sas: phy-12:4 added to port-12:0, phy_mask:0x10 (50010860002f5644) > [ 54.449019] sas: DOING DISCOVERY on port 0, pid:423 > [ 54.461108] sas: Enter sas_scsi_recover_host busy: 0 failed: 0 > [ 54.465601] sas: ata13: end_device-12:0: dev error handler > [ 54.618588] ata13.00: ATA-11: WDC WUH721818ALN604, PCGNW232, max UDMA/133 > [ 56.295486] ata13.00: 4394582016 sectors, multi 0: LBA48 NCQ (depth 32) > [ 56.306849] ata13.00: Features: NCQ-sndrcv NCQ-prio > [ 56.320849] ata13.00: configured for UDMA/133 > [ 56.324504] sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 0 tries: 1 > [ 56.340346] scsi 12:0:0:0: Direct-Access ATA WDC WUH721818AL W232 > PQ: 0 ANSI: 5 > [ 56.347160] sas: DONE DISCOVERY on port 0, pid:423, result:0 > [ 56.348161] sas: phy-12:5 added to port-12:1, phy_mask:0x20 (50010860002f5645) > [ 56.351291] sas: DOING DISCOVERY on port 1, pid:423 > [ 56.363285] sas: Enter sas_scsi_recover_host busy: 0 failed: 0 > [ 56.368788] sas: ata13: end_device-12:0: dev error handler > [ 56.368859] sas: ata14: end_device-12:1: dev error handler > [ 56.522903] ata14.00: ATA-11: WDC WUH721818ALN604, PCGNWTW2, max UDMA/133 > [ 58.402015] ata14.00: 4394582016 sectors, multi 0: LBA48 NCQ (depth 32) > [ 58.406207] ata14.00: Features: NCQ-sndrcv NCQ-prio > [ 58.420254] ata14.00: configured for UDMA/133 > [ 58.423222] sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 0 tries: 1 > [ 58.438869] scsi 12:0:1:0: Direct-Access ATA WDC WUH721818AL WTW2 > PQ: 0 ANSI: 5 > [ 58.445619] sas: DONE DISCOVERY on port 1, pid:423, result:0 > [ 58.446687] sas: phy-12:6 added to port-12:2, phy_mask:0x40 (50010860002f5646) > [ 58.449803] sas: DOING DISCOVERY on port 2, pid:423 > [ 58.461732] sas: Enter sas_scsi_recover_host busy: 0 failed: 0 > [ 58.465369] sas: ata13: end_device-12:0: dev error handler > [ 58.465426] sas: ata14: end_device-12:1: dev error handler > [ 58.465432] sas: ata15: end_device-12:2: dev error handler > [ 58.622345] ata15.00: ATA-12: WDC WUH722222ALN604, LNGNW1TZ, max UDMA/133 > [ 59.540468] ata15.00: 5371330560 sectors, multi 0: LBA48 NCQ (depth 32) > [ 59.588217] ata15.00: Features: NCQ-sndrcv NCQ-prio CDL > [ 59.697588] ata15.00: configured for UDMA/133 > [ 59.700514] sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 0 tries: 1 > [ 59.716067] scsi 12:0:2:0: Direct-Access ATA WDC WUH722222AL W1TZ > PQ: 0 ANSI: 5 > [ 59.723838] sas: DONE DISCOVERY on port 2, pid:423, result:0 > [ 59.724893] sas: phy-12:7 added to port-12:3, phy_mask:0x80 (50010860002f5647) > [ 59.727852] sas: DOING DISCOVERY on port 3, pid:423 > [ 59.739699] sas: Enter sas_scsi_recover_host busy: 0 failed: 0 > [ 59.743344] sas: ata13: end_device-12:0: dev error handler > [ 59.743402] sas: ata14: end_device-12:1: dev error handler > [ 59.743405] sas: ata15: end_device-12:2: dev error handler > [ 59.743429] sas: ata16: end_device-12:3: dev error handler > [ 59.898839] ata16.00: ATA-11: WDC WSH722020ALN604, PCGMW803, max UDMA/133 > [ 62.348080] ata16.00: 4882956288 sectors, multi 0: LBA48 NCQ (depth 32) > [ 62.353731] ata16.00: Features: NCQ-sndrcv NCQ-prio > [ 62.370011] ata16.00: configured for UDMA/133 > [ 62.373532] sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 0 tries: 1 > [ 62.389238] scsi 12:0:3:0: Direct-Access-ZBC ATA WDC WSH722020AL W803 > PQ: 0 ANSI: 7 > [ 62.396642] sas: DONE DISCOVERY on port 3, pid:423, result:0 > > But no scsi disk added: > > # lsscsi > [12:0:0:0] disk ATA WDC WUH721818AL W232 - > [12:0:1:0] disk ATA WDC WUH721818AL WTW2 - > [12:0:2:0] disk ATA WDC WUH722222AL W1TZ - > [12:0:3:0] zbc ATA WDC WSH722020AL W803 - > > (no /dev/sdX device). > While not printed on this run, I often see messages like > > scsi 12:0:0:0: deferred probe error > > No idea why... I do not see where that comes from. It might be related to what I mention, above. Anyway, I would rather you don't get this series bogged down in libsas issues. > > This is all with pm8001 driver. Thanks, John
On 9/12/23 17:49, John Garry wrote: >> +int sas_ata_slave_configure(struct scsi_device *sdev) >> +{ >> + struct domain_device *dev = sdev_to_domain_dev(sdev); >> + struct ata_port *ap = dev->sata_dev.ap; >> + struct device *sdev_dev = &sdev->sdev_gendev; >> + struct device_link *link; >> + >> + ata_sas_slave_configure(sdev, ap); >> + >> + link = device_link_add(sdev_dev, &ap->tdev, >> + DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE); > > I am not sure what is the point of this. For libsas/pm8001 driver, there > is no ata_port -> .. -> host dev link, right? If so, it just seems that > you are are adding a dependency to a device (&ap->tdev) which has no > dependency to a real device itself. For libata, the point is to have PM order suspend resume operations correctly: suspend: scsi dev first, then ata_port resume: ata_port first then scsi dev Strictly speaking, we should do that with ata_dev and scsi dev, but libata PM operations are defined for ata_port, not ata_dev. For libsas, the PM operations come from scsi, so scsi bus operations for devices. And given that: 1) we should already have the dev chain: hba dev -> scsi host ->scsi target -> scsi dev 2) libsas does its own ata PM control when suspending & resuming the HBA (if I understand things correctly) we should thus not need anything special for libsas. ata devices suspend/resume will be done in the right order without an extra link. I do not really understand why hisi_sas need to create that link. If it is indeed needed, then we probably should have it created always by libsas generic code...
+ On 12/09/2023 10:00, Damien Le Moal wrote: >>> +int sas_ata_slave_configure(struct scsi_device *sdev) >>> +{ >>> + struct domain_device *dev = sdev_to_domain_dev(sdev); >>> + struct ata_port *ap = dev->sata_dev.ap; >>> + struct device *sdev_dev = &sdev->sdev_gendev; >>> + struct device_link *link; >>> + >>> + ata_sas_slave_configure(sdev, ap); >>> + >>> + link = device_link_add(sdev_dev, &ap->tdev, >>> + DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE); >> I am not sure what is the point of this. For libsas/pm8001 driver, there >> is no ata_port -> .. -> host dev link, right? If so, it just seems that >> you are are adding a dependency to a device (&ap->tdev) which has no >> dependency to a real device itself. > For libata, the point is to have PM order suspend resume operations correctly: > suspend: scsi dev first, then ata_port > resume: ata_port first then scsi dev > > Strictly speaking, we should do that with ata_dev and scsi dev, but libata PM > operations are defined for ata_port, not ata_dev. > > For libsas, the PM operations come from scsi, so scsi bus operations for > devices. And given that: > 1) we should already have the dev chain: > hba dev -> scsi host ->scsi target -> scsi dev That would seem right, since scsi_add_host() is passed the host dev and the ancestry would be created naturally, but for hisi_sas a link was still required. Let me check why that is again ... > 2) libsas does its own ata PM control when suspending & resuming the HBA (if I > understand things correctly) > > we should thus not need anything special for libsas. ata devices suspend/resume > will be done in the right order without an extra link. I do not really > understand why hisi_sas need to create that link. If it is indeed needed, then > we probably should have it created always by libsas generic code... Thanks, John
On Mon, Sep 11, 2023 at 01:02:01PM +0900, 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> Tested-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
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> --- 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(-)