| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 8 Aug 2018 08:47:21 +0000
From: Stanislav Nijnikov <Stanislav.Nijnikov@....com>
To: Evan Green <evgreen@...omium.org>
CC: Vinayak Holikatti <vinholikatti@...il.com>,
"jejb@...ux.vnet.ibm.com" <jejb@...ux.vnet.ibm.com>,
"martin.petersen@...cle.com" <martin.petersen@...cle.com>,
"adrian.hunter@...el.com" <adrian.hunter@...el.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
Bart Van Assche <Bart.VanAssche@....com>
Subject: RE: [PATCH v3] scsi: ufs: Make sysfs attributes writable
Hi Evan,
> -----Original Message-----
> From: Evan Green <evgreen@...omium.org>
> Sent: Tuesday, August 7, 2018 9:15 PM
> To: Stanislav Nijnikov <Stanislav.Nijnikov@....com>
> Cc: Vinayak Holikatti <vinholikatti@...il.com>; jejb@...ux.vnet.ibm.com; martin.petersen@...cle.com; adrian.hunter@...el.com;
> linux-kernel@...r.kernel.org; linux-scsi@...r.kernel.org; Bart Van Assche <Bart.VanAssche@....com>
> Subject: Re: [PATCH v3] scsi: ufs: Make sysfs attributes writable
>
> Hi Stanislav. Thanks for the review.
>
> On Mon, Aug 6, 2018 at 2:28 AM Stanislav Nijnikov
> <Stanislav.Nijnikov@....com> wrote:
> >
> > Hi Evan,
> >
> > > -----Original Message-----
> > > From: Evan Green <evgreen@...omium.org>
> > > Sent: Wednesday, July 25, 2018 11:15 PM
> > > To: Vinayak Holikatti <vinholikatti@...il.com>; James E.J. Bottomley <jejb@...ux.vnet.ibm.com>; Martin K. Petersen
> > > <martin.petersen@...cle.com>; Stanislav Nijnikov <Stanislav.Nijnikov@....com>; Adrian Hunter <adrian.hunter@...el.com>;
> linux-
> > > kernel@...r.kernel.org; linux-scsi@...r.kernel.org; Bart Van Assche <Bart.VanAssche@....com>
> > > Cc: Evan Green <evgreen@...omium.org>
> > > Subject: [PATCH v3] scsi: ufs: Make sysfs attributes writable
> > >
> > > This change makes the UFS controller's sysfs attributes writable, which
> > > will enable users to modify attributes. This can be useful during factory
> > > provisioning for setting up critical attributes like the reference clock
> > > frequency.
> > >
> > > Signed-off-by: Evan Green <evgreen@...omium.org>
> > > ---
> > > Configfs was determined to be the preferred mechanism for writing the
> > > config descriptor, but attributes also need to be written during setup,
> > > and are already present in sysfs. Making these attributes writable is
> > > also helpful for debugging and experimentation.
> > >
> > > Changes since v2:
> > > - Removed the configuration descriptor changes from the series,
> > > since configfs was the preferred way to write to that, leaving only
> > > this change.
> > >
> > > Changes since v1:
> > > - Reworked the interface to show each unit of the config
> > > descriptor as a separate directory, rather than the previous method I
> > > had of a file for selecting the unit, and then a common set of files
> > > that interacted with whichever unit was selected. I did some kobject
> > > magic to accomplish this. I noticed from Greg KH's reply to Sayali's
> > > patches [1] that configfs might be the preferred method. Let me know
> > > if I should abandon this series in favor of Sayali's, with the
> > > possible exception of "Make sysfs attributes writable".
> > > - Squashed documentation changes into their respective code
> > > changes.
> > > - I decided to keep the config descriptor attributes as their
> > > own files, rather than hiding writes behind device descriptor and unit
> > > descriptor, as I think that's more future proof and true to the UFS spec.
> > >
> > > [1] https://lkml.org/lkml/2018/6/8/210
> > >
> > > Documentation/ABI/testing/sysfs-driver-ufs | 17 +--------
> > > drivers/scsi/ufs/ufs-sysfs.c | 58 ++++++++++++++++++++----------
> > > 2 files changed, 40 insertions(+), 35 deletions(-)
> > >
> ...
> > > diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> > > index 8d9332bb7d0c..5e286b9d1aea 100644
> > > --- a/drivers/scsi/ufs/ufs-sysfs.c
> > > +++ b/drivers/scsi/ufs/ufs-sysfs.c
> > > @@ -655,7 +655,7 @@ static const struct attribute_group ufs_sysfs_flags_group = {
> > > .attrs = ufs_sysfs_device_flags,
> > > };
> > >
> > > -#define UFS_ATTRIBUTE(_name, _uname) \
> > > +#define UFS_ATTRIBUTE_SHOW(_name, _uname) \
> > > static ssize_t _name##_show(struct device *dev, \
> > > struct device_attribute *attr, char *buf) \
> > > { \
> > > @@ -665,25 +665,45 @@ static ssize_t _name##_show(struct device *dev, \
> > > QUERY_ATTR_IDN##_uname, 0, 0, &value)) \
> > > return -EINVAL; \
> > > return sprintf(buf, "0x%08X\n", value); \
> > > -} \
> > > -static DEVICE_ATTR_RO(_name)
> > > +}
> > >
> > > -UFS_ATTRIBUTE(boot_lun_enabled, _BOOT_LU_EN);
> > > -UFS_ATTRIBUTE(current_power_mode, _POWER_MODE);
> > > -UFS_ATTRIBUTE(active_icc_level, _ACTIVE_ICC_LVL);
> > > -UFS_ATTRIBUTE(ooo_data_enabled, _OOO_DATA_EN);
> > > -UFS_ATTRIBUTE(bkops_status, _BKOPS_STATUS);
> > > -UFS_ATTRIBUTE(purge_status, _PURGE_STATUS);
> > > -UFS_ATTRIBUTE(max_data_in_size, _MAX_DATA_IN);
> > > -UFS_ATTRIBUTE(max_data_out_size, _MAX_DATA_OUT);
> > > -UFS_ATTRIBUTE(reference_clock_frequency, _REF_CLK_FREQ);
> > > -UFS_ATTRIBUTE(configuration_descriptor_lock, _CONF_DESC_LOCK);
> > > -UFS_ATTRIBUTE(max_number_of_rtt, _MAX_NUM_OF_RTT);
> > > -UFS_ATTRIBUTE(exception_event_control, _EE_CONTROL);
> > > -UFS_ATTRIBUTE(exception_event_status, _EE_STATUS);
> > > -UFS_ATTRIBUTE(ffu_status, _FFU_STATUS);
> > > -UFS_ATTRIBUTE(psa_state, _PSA_STATE);
> > > -UFS_ATTRIBUTE(psa_data_size, _PSA_DATA_SIZE);
> > > +#define UFS_ATTRIBUTE_RO(_name, _uname) \
> > > +UFS_ATTRIBUTE_SHOW(_name, _uname) \
> > > +DEVICE_ATTR_RO(_name)
> > It should be static here.
>
> Will fix.
>
> >
> > > +
> > > +#define UFS_ATTRIBUTE_RW(_name, _uname) \
> > > +UFS_ATTRIBUTE_SHOW(_name, _uname) \
> > > +static ssize_t _name##_store(struct device *dev, \
> > > + struct device_attribute *attr, const char *buf, \
> > > + size_t count) \
> > > +{ \
> > > + struct ufs_hba *hba = dev_get_drvdata(dev); \
> > > + u32 value; \
> > > + if (kstrtou32(buf, 0, &value)) \
> > > + return -EINVAL; \
> > > + if (ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR, \
> > > + QUERY_ATTR_IDN##_uname, 0, 0, &value)) \
> > > + return -EINVAL; \
> > > + return count; \
> > > +} \
> > > +static DEVICE_ATTR_RW(_name)
> > > +
> > > +UFS_ATTRIBUTE_RW(boot_lun_enabled, _BOOT_LU_EN);
> > > +UFS_ATTRIBUTE_RO(current_power_mode, _POWER_MODE);
> > > +UFS_ATTRIBUTE_RW(active_icc_level, _ACTIVE_ICC_LVL);
> > > +UFS_ATTRIBUTE_RW(ooo_data_enabled, _OOO_DATA_EN);
> > I would prefer to leave "write once" attributes as read-only.
>
> Oh, but I want those write once attributes, I plan to use them during
> provisioning. Are you worried about accidental writes? My mind jumps
> to some sort of unlock mechanism where you write a magic string into
> an additional sysfs file to unlock the write-once attributes. But the
> last time I proposed a sysfs file that affected the behavior of other
> sysfs files, I got the proverbial raspberry. Any thoughts?
>
Well, I suppose users with root permissions should know what they are
doing. At least, add a special comment for these in the ABI file, not
everyone has access to the UFS spec.
> >
> > > +UFS_ATTRIBUTE_RO(bkops_status, _BKOPS_STATUS);
> > > +UFS_ATTRIBUTE_RO(purge_status, _PURGE_STATUS);
> > > +UFS_ATTRIBUTE_RW(max_data_in_size, _MAX_DATA_IN);
> > > +UFS_ATTRIBUTE_RW(max_data_out_size, _MAX_DATA_OUT);
> > > +UFS_ATTRIBUTE_RW(reference_clock_frequency, _REF_CLK_FREQ);
> > > +UFS_ATTRIBUTE_RW(configuration_descriptor_lock, _CONF_DESC_LOCK);
> > Same here, "write once" attribute.
> >
> > > +UFS_ATTRIBUTE_RW(max_number_of_rtt, _MAX_NUM_OF_RTT);
> > > +UFS_ATTRIBUTE_RW(exception_event_control, _EE_CONTROL);
> > > +UFS_ATTRIBUTE_RW(exception_event_status, _EE_STATUS);
> > This one is read only attribute.
>
> Will fix.
>
> >
> > > +UFS_ATTRIBUTE_RO(ffu_status, _FFU_STATUS);
> > > +UFS_ATTRIBUTE_RO(psa_state, _PSA_STATE);
> > > +UFS_ATTRIBUTE_RO(psa_data_size, _PSA_DATA_SIZE);
> > >
> > > static struct attribute *ufs_sysfs_attributes[] = {
> > > &dev_attr_boot_lun_enabled.attr,
> > > --
> > > 2.16.4
> >
> > I would add some write option to some flags as well. For example, enabling/
> > disabling the background operations could be very useful.
>
> I agree. Okay if I do that as a follow-on patch?
Sure.
>
> >
Regards
Stanislav
Powered by blists - more mailing lists