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

[Evan Green]

On Wed, Jun 13, 2018 at 3:12 AM Stanislav Nijnikov
<Stanislav.Nijnikov@xxxxxxx> wrote:
>
>
>
> > -----Original Message-----
> > From: linux-scsi-owner@xxxxxxxxxxxxxxx <linux-scsi-owner@xxxxxxxxxxxxxxx> On Behalf Of Evan Green
> > Sent: Tuesday, June 12, 2018 10:43 PM
> > To: Stanislav Nijnikov <Stanislav.Nijnikov@xxxxxxx>
> > Cc: adrian.hunter@xxxxxxxxx; Vinayak Holikatti <vinholikatti@xxxxxxxxx>; jejb@xxxxxxxxxxxxxxxxxx; martin.petersen@xxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx; Gwendal Grignou <gwendal@xxxxxxxxxxxx>; Alex Lemberg
> > <Alex.Lemberg@xxxxxxx>; Avri Altman <Avri.Altman@xxxxxxx>
> > Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> >
> > On Sun, Jun 10, 2018 at 2:31 AM Stanislav Nijnikov
> > <Stanislav.Nijnikov@xxxxxxx> wrote:
> > >
> > > Hi Adrian,
> > >
> > > > -----Original Message-----
> > > > From: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> > > > Sent: Friday, June 8, 2018 3:31 PM
> > > > To: Evan Green <evgreen@xxxxxxxxxxxx>; Stanislav Nijnikov <Stanislav.Nijnikov@xxxxxxx>
> > > > Cc: Vinayak Holikatti <vinholikatti@xxxxxxxxx>; jejb@xxxxxxxxxxxxxxxxxx; martin.petersen@xxxxxxxxxx; linux-
> > > > kernel@xxxxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx; Gwendal Grignou <gwendal@xxxxxxxxxxxx>; Alex Lemberg
> > > > <Alex.Lemberg@xxxxxxx>; Avri Altman <Avri.Altman@xxxxxxx>
> > > > 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
> > > > > <Stanislav.Nijnikov@xxxxxxx> wrote:
> > > > >>
> > > > >>
> > > > >>
> > > > >>> -----Original Message-----
> > > > >>> From: linux-scsi-owner@xxxxxxxxxxxxxxx <linux-scsi-owner@xxxxxxxxxxxxxxx> On Behalf Of Evan Green
> > > > >>> Sent: Friday, June 1, 2018 5:44 PM
> > > > >>> To: Stanislav Nijnikov <Stanislav.Nijnikov@xxxxxxx>
> > > > >>> Cc: Vinayak Holikatti <vinholikatti@xxxxxxxxx>; jejb@xxxxxxxxxxxxxxxxxx; martin.petersen@xxxxxxxxxx; linux-
> > > > >>> kernel@xxxxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx; Gwendal Grignou <gwendal@xxxxxxxxxxxx>; Alex Lemberg
> > > > >>> <Alex.Lemberg@xxxxxxx>; Avri Altman <Avri.Altman@xxxxxxx>
> > > > >>> 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
> > > > >>> <Stanislav.Nijnikov@xxxxxxx> 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