lkml.org 
[lkml]   [2019]   [Jun]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] drivers/ata: print trim features at device initialization
From
Date


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);
> }
>

\
 
 \ /
  Last update: 2019-06-14 15:50    [W:0.052 / U:0.572 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site