Re: [PATCH v2 03/21] ata: libata-scsi: link ata port and scsi device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Damien,

On Thu, Sep 14, 2023 at 3:18 PM Damien Le Moal <dlemoal@xxxxxxxxxx> wrote:
> On 9/14/23 16:08, Geert Uytterhoeven wrote:
> > On Wed, Sep 13, 2023 at 12:27 PM Geert Uytterhoeven
> > <geert@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> >>> Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> >>> Reviewed-by: Hannes Reinecke <hare@xxxxxxx>
> >>
> >> 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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux