by Stanislav Nijnikov

[permalink] [raw]

Subject: RE: [PATCH 0/7] Enable UFS provisioning via Linux

> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf Of Evan Green
> Sent: Tuesday, June 12, 2018 10:43 PM
> To: Stanislav Nijnikov <[email protected]>
> Cc: [email protected]; Vinayak Holikatti <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; Gwendal Grignou <[email protected]>; Alex Lemberg
> <[email protected]>; Avri Altman <[email protected]>
> Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
>
> On Sun, Jun 10, 2018 at 2:31 AM Stanislav Nijnikov
> <[email protected]> wrote:
> >
> > Hi Adrian,
> >
> > > -----Original Message-----
> > > From: Adrian Hunter <[email protected]>
> > > Sent: Friday, June 8, 2018 3:31 PM
> > > To: Evan Green <[email protected]>; Stanislav Nijnikov <[email protected]>
> > > Cc: Vinayak Holikatti <[email protected]>; [email protected]; [email protected]; linux-
> > > [email protected]; [email protected]; Gwendal Grignou <[email protected]>; Alex Lemberg
> > > <[email protected]>; Avri Altman <[email protected]>
> > > Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> > >
> > > On 04/06/18 17:59, Evan Green wrote:
> > > > On Sun, Jun 3, 2018 at 3:21 AM Stanislav Nijnikov
> > > > <[email protected]> wrote:
> > > >>
> > > >>
> > > >>
> > > >>> -----Original Message-----
> > > >>> From: [email protected] <[email protected]> On Behalf Of Evan Green
> > > >>> Sent: Friday, June 1, 2018 5:44 PM
> > > >>> To: Stanislav Nijnikov <[email protected]>
> > > >>> Cc: Vinayak Holikatti <[email protected]>; [email protected]; [email protected]; linux-
> > > >>> [email protected]; [email protected]; Gwendal Grignou <[email protected]>; Alex Lemberg
> > > >>> <[email protected]>; Avri Altman <[email protected]>
> > > >>> Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> > > >>>
> > > >>> Hi Stanislav. Thanks for taking a look. Responses below.
> > > >>>
> > > >>> On Thu, May 31, 2018 at 3:04 AM Stanislav Nijnikov
> > > >>> <[email protected]> wrote:
> > > >>>>
> > > >>>> Hi Evan,
> > > >>>> I have some generic notes:
> > > >>>> - Why to create new sysfs entries for the configuration descriptor fields if they are just duplication of fields in the device and
> unit
> > > >>> descriptors? And the sysfs representation of the device and unit descriptors is existing already.
> > > >>>
> > > >>> Well, UFS describes them as different descriptors. I worry that if I
> > > >>> add a bunch of clever logic to hide the config descriptor behind other
> > > >>> descriptors, there might be trouble later if 1) there is a quirky
> > > >>> device that doesn't reflect the values between descriptors quite the
> > > >>> same way or at the same time, or 2) if a later UFS spec adds more
> > > >>> configuration descriptor fields that don't exactly reflect into other
> > > >>> non-config descriptors, the cleverness will look awkward.
> > > >>
> > > >> No additional logic will be required to attach write functionality to the
> > > >> existing entries instead of new defined ones. It will reduce the patch
> > > >> size significantly. And there will be no need for the unit selector
> > > >> mechanism which I'm not sure will be accepted by the SCSI community.
> > > >>
> > > >
> > > > So this would be modifying the existing sysfs entries so that reads
> > > > still come from the device and unit descriptors, but writes go to
> > > > equivalent fields in the config descriptor? I can explore that
> > > > approach. Alternatively, if the unit selector mechanism is not
> > > > desired, I could dynamically create sysfs directories for each unit in
> > > > the config descriptor, but still bring out the config descriptor
> > > > values as separate entries. (I still worry a bit about smashing the
> > > > descriptors together as the UFS spec called them out as different).
> > >
> > > If you use the unit attributes, how do you configure units that do not yet
> > > exist?
> >
> > For example by adding the enable_lun writeable sysfs entry. I think both ways are
> > viable and there are several pitfalls in each of them. Now it's up to Evan to decide
> > how to implement this.
> >
> > >
> > > Perhaps it is better to represent the configuration descriptors exactly as
> > > they are defined in the specification. Probably not worth exposing them at
> > > all if the configuration is locked (attribute bConfigDescrLock == 1).
> > >
> > > Note also that the 2.1 spec. defines bConfDescContinue which allows updates
> > > to be grouped and committed together.
> >
> > The only question is how many devices are ready to get dozens of configuration
> > descriptors related to first eight LUNs instead just one when this lock is enabled.
> >
> > Regards
> > Stanislav
>
> Actually I could use some advice on this. It seems like folks are
> opposed to the idea of having a cfg_unit file, whose value determines
> which index to talk to in the unit_* files. (I personally liked that
> approach, as it was simple, has precedence, and fit the requirements,
> but oh well). My instinct favors Adrian's approach of keeping the
> configuration descriptor separate, rather than hiding it behind the
> device and unit descriptors, as I think it's more true to the UFS spec
> and less likely to cause problems in the future. However I'm trying to
> figure out the best way to do that.
>
> What I _want_ to do is basically create N sysfs groups, where each
> group points to the same array of attributes. Then in the show/store
> methods, look up which group I'm in and use that as an index. But the
> show/store functions only pass the attributes themselves, and there
> seems to be no way for me to get the parent node. So my next plan is
> to create a wrapper around struct device_attribute where I can store
> my index, create a template of attributes, and then create N copies of
> this template. The show/store method is then a single method, which
> uses container_of on the attribute to get the index, offset, and size
> of the descriptor to change. This seems less than ideal to me, as it's
> never fun to feel like you're wasting memory, even though it's
> probably on the order of a kilobyte or two.
>
> Stanislav, you've got the unit descriptors off in the scsi_device,
> which would make a lot of sense for me too, except that I need to
> configure luns that may not exist yet. Can you expand on your
> enable_lun idea?
> -Evan

It's a writeable sysfs entry that receive an integer value (as an index
of a lun that should be enable). The store function does some sanity
checks, reads the configuration descriptor, update the specified
lun enable parameter and sends it. After restart the lun will be available
and ready for further configuration.

Stanislav

2018-06-15 21:20:52

by Evan Green

[permalink] [raw]

Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux

On Wed, Jun 13, 2018 at 3:12 AM Stanislav Nijnikov
<[email protected]> wrote:
>
>
>
> > -----Original Message-----
> > From: [email protected] <[email protected]> On Behalf Of Evan Green
> > Sent: Tuesday, June 12, 2018 10:43 PM
> > To: Stanislav Nijnikov <[email protected]>
> > Cc: [email protected]; Vinayak Holikatti <[email protected]>; [email protected]; [email protected];
> > [email protected]; [email protected]; Gwendal Grignou <[email protected]>; Alex Lemberg
> > <[email protected]>; Avri Altman <[email protected]>
> > Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> >
> > On Sun, Jun 10, 2018 at 2:31 AM Stanislav Nijnikov
> > <[email protected]> wrote:
> > >
> > > Hi Adrian,
> > >
> > > > -----Original Message-----
> > > > From: Adrian Hunter <[email protected]>
> > > > Sent: Friday, June 8, 2018 3:31 PM
> > > > To: Evan Green <[email protected]>; Stanislav Nijnikov <[email protected]>
> > > > Cc: Vinayak Holikatti <[email protected]>; [email protected]; [email protected]; linux-
> > > > [email protected]; [email protected]; Gwendal Grignou <[email protected]>; Alex Lemberg
> > > > <[email protected]>; Avri Altman <[email protected]>
> > > > Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> > > >
> > > > On 04/06/18 17:59, Evan Green wrote:
> > > > > On Sun, Jun 3, 2018 at 3:21 AM Stanislav Nijnikov
> > > > > <[email protected]> wrote:
> > > > >>
> > > > >>
> > > > >>
> > > > >>> -----Original Message-----
> > > > >>> From: [email protected] <[email protected]> On Behalf Of Evan Green
> > > > >>> Sent: Friday, June 1, 2018 5:44 PM
> > > > >>> To: Stanislav Nijnikov <[email protected]>
> > > > >>> Cc: Vinayak Holikatti <[email protected]>; [email protected]; [email protected]; linux-
> > > > >>> [email protected]; [email protected]; Gwendal Grignou <[email protected]>; Alex Lemberg
> > > > >>> <[email protected]>; Avri Altman <[email protected]>
> > > > >>> Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> > > > >>>
> > > > >>> Hi Stanislav. Thanks for taking a look. Responses below.
> > > > >>>
> > > > >>> On Thu, May 31, 2018 at 3:04 AM Stanislav Nijnikov
> > > > >>> <[email protected]> wrote:
> > > > >>>>
> > > > >>>> Hi Evan,
> > > > >>>> I have some generic notes:
> > > > >>>> - Why to create new sysfs entries for the configuration descriptor fields if they are just duplication of fields in the device and
> > unit
> > > > >>> descriptors? And the sysfs representation of the device and unit descriptors is existing already.
> > > > >>>
> > > > >>> Well, UFS describes them as different descriptors. I worry that if I
> > > > >>> add a bunch of clever logic to hide the config descriptor behind other
> > > > >>> descriptors, there might be trouble later if 1) there is a quirky
> > > > >>> device that doesn't reflect the values between descriptors quite the
> > > > >>> same way or at the same time, or 2) if a later UFS spec adds more
> > > > >>> configuration descriptor fields that don't exactly reflect into other
> > > > >>> non-config descriptors, the cleverness will look awkward.
> > > > >>
> > > > >> No additional logic will be required to attach write functionality to the
> > > > >> existing entries instead of new defined ones. It will reduce the patch
> > > > >> size significantly. And there will be no need for the unit selector
> > > > >> mechanism which I'm not sure will be accepted by the SCSI community.
> > > > >>
> > > > >
> > > > > So this would be modifying the existing sysfs entries so that reads
> > > > > still come from the device and unit descriptors, but writes go to
> > > > > equivalent fields in the config descriptor? I can explore that
> > > > > approach. Alternatively, if the unit selector mechanism is not
> > > > > desired, I could dynamically create sysfs directories for each unit in
> > > > > the config descriptor, but still bring out the config descriptor
> > > > > values as separate entries. (I still worry a bit about smashing the
> > > > > descriptors together as the UFS spec called them out as different).
> > > >
> > > > If you use the unit attributes, how do you configure units that do not yet
> > > > exist?
> > >
> > > For example by adding the enable_lun writeable sysfs entry. I think both ways are
> > > viable and there are several pitfalls in each of them. Now it's up to Evan to decide
> > > how to implement this.
> > >
> > > >
> > > > Perhaps it is better to represent the configuration descriptors exactly as
> > > > they are defined in the specification. Probably not worth exposing them at
> > > > all if the configuration is locked (attribute bConfigDescrLock == 1).
> > > >
> > > > Note also that the 2.1 spec. defines bConfDescContinue which allows updates
> > > > to be grouped and committed together.
> > >
> > > The only question is how many devices are ready to get dozens of configuration
> > > descriptors related to first eight LUNs instead just one when this lock is enabled.
> > >
> > > Regards
> > > Stanislav
> >
> > Actually I could use some advice on this. It seems like folks are
> > opposed to the idea of having a cfg_unit file, whose value determines
> > which index to talk to in the unit_* files. (I personally liked that
> > approach, as it was simple, has precedence, and fit the requirements,
> > but oh well). My instinct favors Adrian's approach of keeping the
> > configuration descriptor separate, rather than hiding it behind the
> > device and unit descriptors, as I think it's more true to the UFS spec
> > and less likely to cause problems in the future. However I'm trying to
> > figure out the best way to do that.
> >
> > What I _want_ to do is basically create N sysfs groups, where each
> > group points to the same array of attributes. Then in the show/store
> > methods, look up which group I'm in and use that as an index. But the
> > show/store functions only pass the attributes themselves, and there
> > seems to be no way for me to get the parent node. So my next plan is
> > to create a wrapper around struct device_attribute where I can store
> > my index, create a template of attributes, and then create N copies of
> > this template. The show/store method is then a single method, which
> > uses container_of on the attribute to get the index, offset, and size
> > of the descriptor to change. This seems less than ideal to me, as it's
> > never fun to feel like you're wasting memory, even though it's
> > probably on the order of a kilobyte or two.
> >
> > Stanislav, you've got the unit descriptors off in the scsi_device,
> > which would make a lot of sense for me too, except that I need to
> > configure luns that may not exist yet. Can you expand on your
> > enable_lun idea?
> > -Evan
>
> It's a writeable sysfs entry that receive an integer value (as an index
> of a lun that should be enable). The store function does some sanity
> checks, reads the configuration descriptor, update the specified
> lun enable parameter and sends it. After restart the lun will be available
> and ready for further configuration.
>
> Stanislav
>
>

Thanks Bart for the advice. I took that and ran with it a little in
the next series, which I've just sent out.
Stanislav, thanks for elaborating. For the next spin of this series I
went with keeping the config descriptor separate, but if everyone
prefers putting the config descriptor behind the device/unit
descriptors, I will keep this solution in mind.
-Evan