Messages in this thread | | | Subject | Re: [PATCH] drivers/ata: print trim features at device initialization | From | Konstantin Khlebnikov <> | Date | Fri, 14 Jun 2019 16:49:37 +0300 |
| |
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); > } >
| |