Hi,
> Expands sysfs boot_lun attribute to be writable. Necessary to enable
> proper support for LUN switching on some UFS devices.
Can you please elaborate why is it necessary?
What use case are you running?
>
> Signed-off-by: Nia Espera <[email protected]>
> ---
> drivers/scsi/ufs/ufs-sysfs.c | 67 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 66 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index 5c405ff7b6ea..7bf5d6c3d0ec 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -1047,6 +1047,71 @@ static inline bool ufshcd_is_wb_attrs(enum attr_idn
> idn)
> idn <= QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE;
> }
>
> +static ssize_t boot_lun_enabled_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> + u32 slot;
> + int ret;
> + u8 index = 0;
> +
> + down(&hba->host_sem);
> + if (!ufshcd_is_user_access_allowed(hba)) {
> + up(&hba->host_sem);
> + return -EBUSY;
> + }
> + if (ufshcd_is_wb_attrs(QUERY_ATTR_IDN_BOOT_LU_EN))
Clearly bBootLunEn is not a WB attribute.
> + index = ufshcd_wb_get_query_index(hba);
> + ufshcd_rpm_get_sync(hba);
> +
> + ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> + QUERY_ATTR_IDN_BOOT_LU_EN, index, 0, &slot);
> +
> + ufshcd_rpm_put_sync(hba);
> + if (ret) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = sysfs_emit(buf, "0x%08X\n", slot);
> +out:
> + up(&hba->host_sem);
> + return ret;
> +}
> +
> +static ssize_t boot_lun_enabled_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> + u32 slot;
> + int ret;
> + u8 index = 0;
> +
> + if (kstrtouint(buf, 0, &slot) < 0)
> + return -EINVAL;
You need to verify that no one set bBootLunEn = 0x0 because the device won't boot.
Better check explicitly that slot != bBootLunEn and its either 1 or 2.
Thanks,
Avri
> +
> + down(&hba->host_sem);
> + if (!ufshcd_is_user_access_allowed(hba)) {
> + up(&hba->host_sem);
> + return -EBUSY;
> + }
> + if (ufshcd_is_wb_attrs(QUERY_ATTR_IDN_BOOT_LU_EN))
> + index = ufshcd_wb_get_query_index(hba);
> + ufshcd_rpm_get_sync(hba);
> +
> + ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> + QUERY_ATTR_IDN_BOOT_LU_EN, index, 0, &slot);
> + ufshcd_rpm_put_sync(hba);
> + if (ret) {
> + ret = -EINVAL;
> + goto out;
> + }
> +out:
> + up(&hba->host_sem);
> + return ret ? ret : count;
> +}
> +
> #define UFS_ATTRIBUTE(_name, _uname) \
> static ssize_t _name##_show(struct device *dev, \
> struct device_attribute *attr, char *buf) \
> @@ -1077,8 +1142,8 @@ out: \
> return ret; \
> } \
> static DEVICE_ATTR_RO(_name)
> +static DEVICE_ATTR_RW(boot_lun_enabled);
>
> -UFS_ATTRIBUTE(boot_lun_enabled, _BOOT_LU_EN);
> UFS_ATTRIBUTE(max_data_size_hpb_single_cmd, _MAX_HPB_SINGLE_CMD);
> UFS_ATTRIBUTE(current_power_mode, _POWER_MODE);
> UFS_ATTRIBUTE(active_icc_level, _ACTIVE_ICC_LVL);
> --
> 2.36.1
Hi,
My usecase is enabling boot slot switching on Android A/B devices, where
the active LUN has to be changed in order to facilitate e.g. dual-booting
Android and mainline Linux. A similar interface is exposed by Android,
albeit
via ioctl. I've tested this patch and confirmed it enabled the necessary
functionality.
On 25/05/2022 21:34, Avri Altman wrote:
> Hi,
>> Expands sysfs boot_lun attribute to be writable. Necessary to enable
>> proper support for LUN switching on some UFS devices.
> Can you please elaborate why is it necessary?
> What use case are you running?
>
>> Signed-off-by: Nia Espera <[email protected]>
>> ---
>> drivers/scsi/ufs/ufs-sysfs.c | 67 +++++++++++++++++++++++++++++++++++-
>> 1 file changed, 66 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
>> index 5c405ff7b6ea..7bf5d6c3d0ec 100644
>> --- a/drivers/scsi/ufs/ufs-sysfs.c
>> +++ b/drivers/scsi/ufs/ufs-sysfs.c
>> @@ -1047,6 +1047,71 @@ static inline bool ufshcd_is_wb_attrs(enum attr_idn
>> idn)
>> idn <= QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE;
>> }
>>
>> +static ssize_t boot_lun_enabled_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct ufs_hba *hba = dev_get_drvdata(dev);
>> + u32 slot;
>> + int ret;
>> + u8 index = 0;
>> +
>> + down(&hba->host_sem);
>> + if (!ufshcd_is_user_access_allowed(hba)) {
>> + up(&hba->host_sem);
>> + return -EBUSY;
>> + }
>> + if (ufshcd_is_wb_attrs(QUERY_ATTR_IDN_BOOT_LU_EN))
> Clearly bBootLunEn is not a WB attribute.
>
>> + index = ufshcd_wb_get_query_index(hba);
>> + ufshcd_rpm_get_sync(hba);
>> +
>> + ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> + QUERY_ATTR_IDN_BOOT_LU_EN, index, 0, &slot);
>> +
>> + ufshcd_rpm_put_sync(hba);
>> + if (ret) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + ret = sysfs_emit(buf, "0x%08X\n", slot);
>> +out:
>> + up(&hba->host_sem);
>> + return ret;
>> +}
>> +
>> +static ssize_t boot_lun_enabled_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct ufs_hba *hba = dev_get_drvdata(dev);
>> + u32 slot;
>> + int ret;
>> + u8 index = 0;
>> +
>> + if (kstrtouint(buf, 0, &slot) < 0)
>> + return -EINVAL;
> You need to verify that no one set bBootLunEn = 0x0 because the device won't boot.
> Better check explicitly that slot != bBootLunEn and its either 1 or 2.
>
> Thanks,
> Avri
>
>> +
>> + down(&hba->host_sem);
>> + if (!ufshcd_is_user_access_allowed(hba)) {
>> + up(&hba->host_sem);
>> + return -EBUSY;
>> + }
>> + if (ufshcd_is_wb_attrs(QUERY_ATTR_IDN_BOOT_LU_EN))
>> + index = ufshcd_wb_get_query_index(hba);
>> + ufshcd_rpm_get_sync(hba);
>> +
>> + ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> + QUERY_ATTR_IDN_BOOT_LU_EN, index, 0, &slot);
>> + ufshcd_rpm_put_sync(hba);
>> + if (ret) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +out:
>> + up(&hba->host_sem);
>> + return ret ? ret : count;
>> +}
>> +
>> #define UFS_ATTRIBUTE(_name, _uname) \
>> static ssize_t _name##_show(struct device *dev, \
>> struct device_attribute *attr, char *buf) \
>> @@ -1077,8 +1142,8 @@ out: \
>> return ret; \
>> } \
>> static DEVICE_ATTR_RO(_name)
>> +static DEVICE_ATTR_RW(boot_lun_enabled);
>>
>> -UFS_ATTRIBUTE(boot_lun_enabled, _BOOT_LU_EN);
>> UFS_ATTRIBUTE(max_data_size_hpb_single_cmd, _MAX_HPB_SINGLE_CMD);
>> UFS_ATTRIBUTE(current_power_mode, _POWER_MODE);
>> UFS_ATTRIBUTE(active_icc_level, _ACTIVE_ICC_LVL);
>> --
>> 2.36.1
>
> Hi,
>
> My usecase is enabling boot slot switching on Android A/B devices, where the
> active LUN has to be changed in order to facilitate e.g. dual-booting Android
> and mainline Linux. A similar interface is exposed by Android, albeit via ioctl. I've
> tested this patch and confirmed it enabled the necessary functionality.
>
> On 25/05/2022 21:34, Avri Altman wrote:
> > Hi,
> >> Expands sysfs boot_lun attribute to be writable. Necessary to enable
> >> proper support for LUN switching on some UFS devices.
> > Can you please elaborate why is it necessary?
> > What use case are you running?
NAK with prejudice.
Thanks,
Avri
> >
> >> Signed-off-by: Nia Espera <[email protected]>
> >> ---
> >> drivers/scsi/ufs/ufs-sysfs.c | 67 +++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 66 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/scsi/ufs/ufs-sysfs.c
> >> b/drivers/scsi/ufs/ufs-sysfs.c index 5c405ff7b6ea..7bf5d6c3d0ec
> >> 100644
> >> --- a/drivers/scsi/ufs/ufs-sysfs.c
> >> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> >> @@ -1047,6 +1047,71 @@ static inline bool ufshcd_is_wb_attrs(enum
> >> attr_idn
> >> idn)
> >> idn <= QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE;
> >> }
> >>
> >> +static ssize_t boot_lun_enabled_show(struct device *dev,
> >> + struct device_attribute *attr,
> >> +char *buf) {
> >> + struct ufs_hba *hba = dev_get_drvdata(dev);
> >> + u32 slot;
> >> + int ret;
> >> + u8 index = 0;
> >> +
> >> + down(&hba->host_sem);
> >> + if (!ufshcd_is_user_access_allowed(hba)) {
> >> + up(&hba->host_sem);
> >> + return -EBUSY;
> >> + }
> >> + if (ufshcd_is_wb_attrs(QUERY_ATTR_IDN_BOOT_LU_EN))
> > Clearly bBootLunEn is not a WB attribute.
> >
> >> + index = ufshcd_wb_get_query_index(hba);
> >> + ufshcd_rpm_get_sync(hba);
> >> +
> >> + ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> >> + QUERY_ATTR_IDN_BOOT_LU_EN, index, 0, &slot);
> >> +
> >> + ufshcd_rpm_put_sync(hba);
> >> + if (ret) {
> >> + ret = -EINVAL;
> >> + goto out;
> >> + }
> >> +
> >> + ret = sysfs_emit(buf, "0x%08X\n", slot);
> >> +out:
> >> + up(&hba->host_sem);
> >> + return ret;
> >> +}
> >> +
> >> +static ssize_t boot_lun_enabled_store(struct device *dev,
> >> + struct device_attribute *attr,
> >> + const char *buf, size_t count)
> >> +{
> >> + struct ufs_hba *hba = dev_get_drvdata(dev);
> >> + u32 slot;
> >> + int ret;
> >> + u8 index = 0;
> >> +
> >> + if (kstrtouint(buf, 0, &slot) < 0)
> >> + return -EINVAL;
> > You need to verify that no one set bBootLunEn = 0x0 because the device won't
> boot.
> > Better check explicitly that slot != bBootLunEn and its either 1 or 2.
> >
> > Thanks,
> > Avri
> >
> >> +
> >> + down(&hba->host_sem);
> >> + if (!ufshcd_is_user_access_allowed(hba)) {
> >> + up(&hba->host_sem);
> >> + return -EBUSY;
> >> + }
> >> + if (ufshcd_is_wb_attrs(QUERY_ATTR_IDN_BOOT_LU_EN))
> >> + index = ufshcd_wb_get_query_index(hba);
> >> + ufshcd_rpm_get_sync(hba);
> >> +
> >> + ret = ufshcd_query_attr_retry(hba,
> UPIU_QUERY_OPCODE_WRITE_ATTR,
> >> + QUERY_ATTR_IDN_BOOT_LU_EN, index, 0, &slot);
> >> + ufshcd_rpm_put_sync(hba);
> >> + if (ret) {
> >> + ret = -EINVAL;
> >> + goto out;
> >> + }
> >> +out:
> >> + up(&hba->host_sem);
> >> + return ret ? ret : count;
> >> +}
> >> +
> >> #define UFS_ATTRIBUTE(_name, _uname) \
> >> static ssize_t _name##_show(struct device *dev, \
> >> struct device_attribute *attr, char *buf) \
> >> @@ -1077,8 +1142,8 @@ out: \
> >> return ret; \
> >> } \
> >> static DEVICE_ATTR_RO(_name)
> >> +static DEVICE_ATTR_RW(boot_lun_enabled);
> >>
> >> -UFS_ATTRIBUTE(boot_lun_enabled, _BOOT_LU_EN);
> >> UFS_ATTRIBUTE(max_data_size_hpb_single_cmd,
> _MAX_HPB_SINGLE_CMD);
> >> UFS_ATTRIBUTE(current_power_mode, _POWER_MODE);
> >> UFS_ATTRIBUTE(active_icc_level, _ACTIVE_ICC_LVL);
> >> --
> >> 2.36.1
On 27/05/2022 07:17, Avri Altman wrote:
>>
>> Hi,
>>
>> My usecase is enabling boot slot switching on Android A/B devices, where the
>> active LUN has to be changed in order to facilitate e.g. dual-booting Android
>> and mainline Linux. A similar interface is exposed by Android, albeit via ioctl. I've
>> tested this patch and confirmed it enabled the necessary functionality.
>>
>> On 25/05/2022 21:34, Avri Altman wrote:
>>> Hi,
>>>> Expands sysfs boot_lun attribute to be writable. Necessary to enable
>>>> proper support for LUN switching on some UFS devices.
>>> Can you please elaborate why is it necessary?
>>> What use case are you running?
> NAK with prejudice.
Hi Avri,
Could you explain why the NAK here? Boot LUN switching is used on a lot
of embedded devices to implement A/B updates, Android devices are just
one such example.
Distributions like postmarketOS [1] aim to support upstream Linux on
mobile devices, particularly those that are no longer supported by the
vendor. Being able to make use of features like A/B updates is something
that I expect more distributions to be considering in the future, as we
start to see more Linux devices with support for features like this.
If safety is a concern, or if the values are device specific, we can
look at protecting the write functionality and configuration behind DT
properties, or coming up with another alternative.
[1]: https://postmarketos.org
Kind regards,
Caleb (they/he)
>
> Thanks,
> Avri
>
>>>
>>>> Signed-off-by: Nia Espera <[email protected]>
>>>> ---
>>>> drivers/scsi/ufs/ufs-sysfs.c | 67 +++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 66 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/scsi/ufs/ufs-sysfs.c
>>>> b/drivers/scsi/ufs/ufs-sysfs.c index 5c405ff7b6ea..7bf5d6c3d0ec
>>>> 100644
>>>> --- a/drivers/scsi/ufs/ufs-sysfs.c
>>>> +++ b/drivers/scsi/ufs/ufs-sysfs.c
>>>> @@ -1047,6 +1047,71 @@ static inline bool ufshcd_is_wb_attrs(enum
>>>> attr_idn
>>>> idn)
>>>> idn <= QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE;
>>>> }
>>>>
>>>> +static ssize_t boot_lun_enabled_show(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> +char *buf) {
>>>> + struct ufs_hba *hba = dev_get_drvdata(dev);
>>>> + u32 slot;
>>>> + int ret;
>>>> + u8 index = 0;
>>>> +
>>>> + down(&hba->host_sem);
>>>> + if (!ufshcd_is_user_access_allowed(hba)) {
>>>> + up(&hba->host_sem);
>>>> + return -EBUSY;
>>>> + }
>>>> + if (ufshcd_is_wb_attrs(QUERY_ATTR_IDN_BOOT_LU_EN))
>>> Clearly bBootLunEn is not a WB attribute.
>>>
>>>> + index = ufshcd_wb_get_query_index(hba);
>>>> + ufshcd_rpm_get_sync(hba);
>>>> +
>>>> + ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>>>> + QUERY_ATTR_IDN_BOOT_LU_EN, index, 0, &slot);
>>>> +
>>>> + ufshcd_rpm_put_sync(hba);
>>>> + if (ret) {
>>>> + ret = -EINVAL;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + ret = sysfs_emit(buf, "0x%08X\n", slot);
>>>> +out:
>>>> + up(&hba->host_sem);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static ssize_t boot_lun_enabled_store(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + const char *buf, size_t count)
>>>> +{
>>>> + struct ufs_hba *hba = dev_get_drvdata(dev);
>>>> + u32 slot;
>>>> + int ret;
>>>> + u8 index = 0;
>>>> +
>>>> + if (kstrtouint(buf, 0, &slot) < 0)
>>>> + return -EINVAL;
>>> You need to verify that no one set bBootLunEn = 0x0 because the device won't
>> boot.
>>> Better check explicitly that slot != bBootLunEn and its either 1 or 2.
>>>
>>> Thanks,
>>> Avri
>>>
>>>> +
>>>> + down(&hba->host_sem);
>>>> + if (!ufshcd_is_user_access_allowed(hba)) {
>>>> + up(&hba->host_sem);
>>>> + return -EBUSY;
>>>> + }
>>>> + if (ufshcd_is_wb_attrs(QUERY_ATTR_IDN_BOOT_LU_EN))
>>>> + index = ufshcd_wb_get_query_index(hba);
>>>> + ufshcd_rpm_get_sync(hba);
>>>> +
>>>> + ret = ufshcd_query_attr_retry(hba,
>> UPIU_QUERY_OPCODE_WRITE_ATTR,
>>>> + QUERY_ATTR_IDN_BOOT_LU_EN, index, 0, &slot);
>>>> + ufshcd_rpm_put_sync(hba);
>>>> + if (ret) {
>>>> + ret = -EINVAL;
>>>> + goto out;
>>>> + }
>>>> +out:
>>>> + up(&hba->host_sem);
>>>> + return ret ? ret : count;
>>>> +}
>>>> +
>>>> #define UFS_ATTRIBUTE(_name, _uname) \
>>>> static ssize_t _name##_show(struct device *dev, \
>>>> struct device_attribute *attr, char *buf) \
>>>> @@ -1077,8 +1142,8 @@ out: \
>>>> return ret; \
>>>> } \
>>>> static DEVICE_ATTR_RO(_name)
>>>> +static DEVICE_ATTR_RW(boot_lun_enabled);
>>>>
>>>> -UFS_ATTRIBUTE(boot_lun_enabled, _BOOT_LU_EN);
>>>> UFS_ATTRIBUTE(max_data_size_hpb_single_cmd,
>> _MAX_HPB_SINGLE_CMD);
>>>> UFS_ATTRIBUTE(current_power_mode, _POWER_MODE);
>>>> UFS_ATTRIBUTE(active_icc_level, _ACTIVE_ICC_LVL);
>>>> --
>>>> 2.36.1
Hi Caleb,
> On 27/05/2022 07:17, Avri Altman wrote:
> >>
> >> Hi,
> >>
> >> My usecase is enabling boot slot switching on Android A/B devices,
> >> where the active LUN has to be changed in order to facilitate e.g.
> >> dual-booting Android and mainline Linux. A similar interface is
> >> exposed by Android, albeit via ioctl. I've tested this patch and confirmed it
> enabled the necessary functionality.
> >>
> >> On 25/05/2022 21:34, Avri Altman wrote:
> >>> Hi,
> >>>> Expands sysfs boot_lun attribute to be writable. Necessary to
> >>>> enable proper support for LUN switching on some UFS devices.
> >>> Can you please elaborate why is it necessary?
> >>> What use case are you running?
> > NAK with prejudice.
> Hi Avri,
>
> Could you explain why the NAK here? Boot LUN switching is used on a lot of
> embedded devices to implement A/B updates, Android devices are just one such
> example.
Sorry for not giving a proper explanation.
As a design rule, sysfs attribute files should not be used to make persistent modifications
to a device configuration. This rule applies to all subsystems and ufs is no different.
>
> Distributions like postmarketOS [1] aim to support upstream Linux on mobile
> devices, particularly those that are no longer supported by the vendor. Being
> able to make use of features like A/B updates is something that I expect more
> distributions to be considering in the future, as we start to see more Linux
> devices with support for features like this.
Changing a UFS device configuration can be done using the ufs-bsg framework.
This framework provides a command passthrough interface with the /dev/ufs-bsgX
bsg node. For examples on how to use this, you can see the ufs-utils project:
https://github.com/westerndigitalcorporation/ufs-utils
Thanks,
Avri
>
> If safety is a concern, or if the values are device specific, we can look at
> protecting the write functionality and configuration behind DT properties, or
> coming up with another alternative.
>
> [1]: https://postmarketos.org
>
> Kind regards,
> Caleb (they/he)
>
> >
> > Thanks,
> > Avri
> >
> >>>
> >>>> Signed-off-by: Nia Espera <[email protected]>
> >>>> ---
> >>>> drivers/scsi/ufs/ufs-sysfs.c | 67
> +++++++++++++++++++++++++++++++++++-
> >>>> 1 file changed, 66 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/scsi/ufs/ufs-sysfs.c
> >>>> b/drivers/scsi/ufs/ufs-sysfs.c index 5c405ff7b6ea..7bf5d6c3d0ec
> >>>> 100644
> >>>> --- a/drivers/scsi/ufs/ufs-sysfs.c
> >>>> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> >>>> @@ -1047,6 +1047,71 @@ static inline bool ufshcd_is_wb_attrs(enum
> >>>> attr_idn
> >>>> idn)
> >>>> idn <= QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE;
> >>>> }
> >>>>
> >>>> +static ssize_t boot_lun_enabled_show(struct device *dev,
> >>>> + struct device_attribute *attr,
> >>>> +char *buf) {
> >>>> + struct ufs_hba *hba = dev_get_drvdata(dev);
> >>>> + u32 slot;
> >>>> + int ret;
> >>>> + u8 index = 0;
> >>>> +
> >>>> + down(&hba->host_sem);
> >>>> + if (!ufshcd_is_user_access_allowed(hba)) {
> >>>> + up(&hba->host_sem);
> >>>> + return -EBUSY;
> >>>> + }
> >>>> + if (ufshcd_is_wb_attrs(QUERY_ATTR_IDN_BOOT_LU_EN))
> >>> Clearly bBootLunEn is not a WB attribute.
> >>>
> >>>> + index = ufshcd_wb_get_query_index(hba);
> >>>> + ufshcd_rpm_get_sync(hba);
> >>>> +
> >>>> + ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> >>>> + QUERY_ATTR_IDN_BOOT_LU_EN, index, 0, &slot);
> >>>> +
> >>>> + ufshcd_rpm_put_sync(hba);
> >>>> + if (ret) {
> >>>> + ret = -EINVAL;
> >>>> + goto out;
> >>>> + }
> >>>> +
> >>>> + ret = sysfs_emit(buf, "0x%08X\n", slot);
> >>>> +out:
> >>>> + up(&hba->host_sem);
> >>>> + return ret;
> >>>> +}
> >>>> +
> >>>> +static ssize_t boot_lun_enabled_store(struct device *dev,
> >>>> + struct device_attribute *attr,
> >>>> + const char *buf, size_t
> >>>> +count) {
> >>>> + struct ufs_hba *hba = dev_get_drvdata(dev);
> >>>> + u32 slot;
> >>>> + int ret;
> >>>> + u8 index = 0;
> >>>> +
> >>>> + if (kstrtouint(buf, 0, &slot) < 0)
> >>>> + return -EINVAL;
> >>> You need to verify that no one set bBootLunEn = 0x0 because the
> >>> device won't
> >> boot.
> >>> Better check explicitly that slot != bBootLunEn and its either 1 or 2.
> >>>
> >>> Thanks,
> >>> Avri
> >>>
> >>>> +
> >>>> + down(&hba->host_sem);
> >>>> + if (!ufshcd_is_user_access_allowed(hba)) {
> >>>> + up(&hba->host_sem);
> >>>> + return -EBUSY;
> >>>> + }
> >>>> + if (ufshcd_is_wb_attrs(QUERY_ATTR_IDN_BOOT_LU_EN))
> >>>> + index = ufshcd_wb_get_query_index(hba);
> >>>> + ufshcd_rpm_get_sync(hba);
> >>>> +
> >>>> + ret = ufshcd_query_attr_retry(hba,
> >> UPIU_QUERY_OPCODE_WRITE_ATTR,
> >>>> + QUERY_ATTR_IDN_BOOT_LU_EN, index, 0, &slot);
> >>>> + ufshcd_rpm_put_sync(hba);
> >>>> + if (ret) {
> >>>> + ret = -EINVAL;
> >>>> + goto out;
> >>>> + }
> >>>> +out:
> >>>> + up(&hba->host_sem);
> >>>> + return ret ? ret : count;
> >>>> +}
> >>>> +
> >>>> #define UFS_ATTRIBUTE(_name, _uname) \
> >>>> static ssize_t _name##_show(struct device *dev, \
> >>>> struct device_attribute *attr, char *buf) \
> >>>> @@ -1077,8 +1142,8 @@ out: \
> >>>> return ret; \
> >>>> } \
> >>>> static DEVICE_ATTR_RO(_name)
> >>>> +static DEVICE_ATTR_RW(boot_lun_enabled);
> >>>>
> >>>> -UFS_ATTRIBUTE(boot_lun_enabled, _BOOT_LU_EN);
> >>>> UFS_ATTRIBUTE(max_data_size_hpb_single_cmd,
> >> _MAX_HPB_SINGLE_CMD);
> >>>> UFS_ATTRIBUTE(current_power_mode, _POWER_MODE);
> >>>> UFS_ATTRIBUTE(active_icc_level, _ACTIVE_ICC_LVL);
> >>>> --
> >>>> 2.36.1
On 6/1/22 10:05, Avri Altman wrote:
> As a design rule, sysfs attribute files should not be used to make
> persistent modifications to a device configuration. This rule applies
> to all subsystems and ufs is no different.
Hmm ... where does that rule come from? I can't find it in
Documentation/admin-guide/sysfs-rules.rst. Did I perhaps overlook something?
Thanks,
Bart.
On 6/5/22 12:55, Bart Van Assche wrote:
> On 6/1/22 10:05, Avri Altman wrote:
>> As a design rule, sysfs attribute files should not be used to make
>> persistent modifications to a device configuration. This rule applies
>> to all subsystems and ufs is no different.
>
> Hmm ... where does that rule come from? I can't find it in
> Documentation/admin-guide/sysfs-rules.rst. Did I perhaps overlook something?
I am not aware of any writable sysfs attribute file that can be used to
make persistent device configuration changes, at least in storage area.
I know of plenty that do change a device setting, but without saving this
setting to maintain it across power cycles. Do you know of any such
attribute ? I was under the impression that sysfs should not be used to
persistently reconfigure a device...
--
Damien Le Moal
Western Digital Research
On 6/5/22 19:48, Damien Le Moal wrote:
> On 6/5/22 12:55, Bart Van Assche wrote:
>> On 6/1/22 10:05, Avri Altman wrote:
>>> As a design rule, sysfs attribute files should not be used to make
>>> persistent modifications to a device configuration. This rule applies
>>> to all subsystems and ufs is no different.
>>
>> Hmm ... where does that rule come from? I can't find it in
>> Documentation/admin-guide/sysfs-rules.rst. Did I perhaps overlook something?
>
> I am not aware of any writable sysfs attribute file that can be used to
> make persistent device configuration changes, at least in storage area.
> I know of plenty that do change a device setting, but without saving this
> setting to maintain it across power cycles. Do you know of any such
> attribute ? I was under the impression that sysfs should not be used to
> persistently reconfigure a device...
I don't think the above is sufficient as an argument to reject a new
patch that introduces a sysfs attribute that changes the device
configuration.
Thanks,
Bart.
On 2022/06/06 22:16, Bart Van Assche wrote:
> On 6/5/22 19:48, Damien Le Moal wrote:
>> On 6/5/22 12:55, Bart Van Assche wrote:
>>> On 6/1/22 10:05, Avri Altman wrote:
>>>> As a design rule, sysfs attribute files should not be used to make
>>>> persistent modifications to a device configuration. This rule applies
>>>> to all subsystems and ufs is no different.
>>>
>>> Hmm ... where does that rule come from? I can't find it in
>>> Documentation/admin-guide/sysfs-rules.rst. Did I perhaps overlook something?
>>
>> I am not aware of any writable sysfs attribute file that can be used to
>> make persistent device configuration changes, at least in storage area.
>> I know of plenty that do change a device setting, but without saving this
>> setting to maintain it across power cycles. Do you know of any such
>> attribute ? I was under the impression that sysfs should not be used to
>> persistently reconfigure a device...
>
> I don't think the above is sufficient as an argument to reject a new
> patch that introduces a sysfs attribute that changes the device
> configuration.
It depends if we can guarantee that the write access to the sysfs file is done
with the same security checks as for a passthrough command issued from user
space. I have not checked.
I would also argue that this particular feature is related to the boot device
management, which is not something we do in the kernel. There is no sysfs
interface to set the bootable flag of a partition on a disk, right ? That is
very similar to me. The kernel should not bother about that kind of interface.
User application tools can deal with that.
>
> Thanks,
>
> Bart.
>
--
Damien Le Moal
Western Digital Research