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 <a5b6@xxxxxxxxxx> > >>>> --- > >>>> 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
