Re: [PATCH] drivers/ata: print trim features at device initialization

[Konstantin Khlebnikov]

On 11.06.2019 1:48, James Bottomley wrote:
> On Mon, 2019-06-10 at 10:49 +0300, Konstantin Khlebnikov wrote:
> > On 10.06.2019 0:37, James Bottomley wrote:
> > > On Sat, 2019-06-08 at 17:13 +0300, Konstantin Khlebnikov wrote:
> > > > > On 08.06.2019 11:25, Christoph Hellwig wrote:> On Fri, Jun 07,
> > > > > 2019
> > > > > at 10:34:39AM +0300, Konstantin Khlebnikov wrote:
> > > > >    >
> > > > >    > Do we really need to spam dmesg with even more ATA
> > > > > crap?  What
> > > > > about
> > > > >    > a sysfs file that can be read on demand instead?
> > > > >    >
> > > > >
> > > > > Makes sense.
> > > > >
> > > > > Trim state is exposed for ata_device:
> > > > > /sys/class/ata_device/devX.Y/trim
> > > > > but there is no link from scsi device to ata device so they
> > > > > hard to match.
> > > > >
> > > > > I'll think about it.
> > > >
> > > > Nope. There is no obvious way to link scsi device with
> > > > ata_device. ata_device is built on top of "transport_class" and
> > > > "attribute_container". This some extremely over engineered sysfs
> > > > framework used only in ata/scsi. I don't want to touch this.
> > >
> > > You don't need to know any of that.  The problem is actually when
> > > the ata transport classes were first created, the devices weren't
> > > properly parented.  What should have happened, like every other
> > > transport class, is that the devices should have descended down to
> > > the scsi device as the leaf in an integrated fashion.  Instead,
> > > what we seem to have is three completely separate trees.
> > >
> > > So if you look at a SAS device, you see from the pci device:
> > >
> > > host2/port-2:0/end_device-2:0/target2:0:0/2:0:0:0/block/sdb/sdb1
> > >
> > > But if you look at a SATA device, you see three separate paths:
> > >
> > > ata3/host3/target3\:0\:0/3\:0\:0\:0/block/sda/sda1
> > > ata3/link3/dev3.0/ata_device/dev3.0
> > > ata3/ata_port/ata3
> > >
> > > Instead of an integrated tree
> > >
> > > Unfortunately, this whole thing is unfixable now.  If I integrate
> > > the tree properly, the separate port and link directories will get
> > > subsumed and we won't be able to recover them with judicious
> > > linking so scripts relying on them will break.  The best we can
> > > probably do is add additional links with what we have.
> > >
> > > To follow the way we usually do it, there should be a link from the
> > > ata device to the scsi target, but that wouldn't help you find the
> > > "trim" files, so it sounds like you want a link from the scsi
> > > device to the ata device, which would?
> >
> > Yes, I'm talking about link from scsi device to leaf ata_device node.
> >
> > In libata scsi_device has one to one relation with ata_device.
> > So making link like /sys/class/block/sda/device/ata_device should be
> > possible easy.
> > But I haven't found implicit reference from struct ata_device to
> > ata_device in sysfs.
>
> If that's all you want, it is pretty simple modulo the fact we can only
> get at the tdev, not the lower transport device, which is what you
> want, but at least it's linear from the symlink.
>
> The attached patch should do this.
>
> Now I see this for my non-sas device:
>
> # ls -l /sys/class/scsi_device/3\:0\:0\:0/device/ata_device
> lrwxrwxrwx 1 root root 0 Jun 10 13:39 /sys/class/scsi_device/3:0:0:0/device/ata_device -> ../../../link3/dev3.0

I've tried this too. Such link is not very useful,
because attribute 'trim' is deeper and suffix path isn't constant:

/sys/class/block/sda/device/ata_device/ata_device/dev1.0/trim

while I expect something like

/sys/class/block/sda/device/ata_device/trim

> James
>
> ---
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 391ac0503dc0..b644336a6d65 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -4576,7 +4576,20 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
>   			sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
>   						 NULL);
>   			if (!IS_ERR(sdev)) {
> +				int r;
> +
>   				dev->sdev = sdev;
> +				/* this is a sysfs stupidity: we don't
> +				 * care if the link actually gets
> +				 * created: there's no error handling
> +				 * for failure and we continue on
> +				 * regardless, but we have to discard
> +				 * the return value like this to
> +				 * defeat unused result checking */
> +				r = sysfs_create_link(&sdev->sdev_gendev.kobj,
> +						      &dev->tdev.kobj,
> +						      "ata_device");
> +				(void)r;
>   				scsi_device_put(sdev);
>   			} else {
>   				dev->sdev = NULL;
> @@ -4703,6 +4716,7 @@ static void ata_scsi_remove_dev(struct ata_device *dev)
>   		ata_dev_info(dev, "detaching (SCSI %s)\n",
>   			     dev_name(&sdev->sdev_gendev));
>   
> +		sysfs_remove_link(&sdev->sdev_gendev.kobj, "ata_device");
>   		scsi_remove_device(sdev);
>   		scsi_device_put(sdev);
>   	}