* [RESEND PATCH] scsi: ufs: sysfs: support writing boot_lun attr @ 2022-05-25 16:40 Nia Espera 2022-05-25 18:34 ` Avri Altman 0 siblings, 1 reply; 10+ messages in thread From: Nia Espera @ 2022-05-25 16:40 UTC (permalink / raw) To: Alim Akhtar, Avri Altman, James E . J . Bottomley, Martin K . Petersen, linux-scsi, linux-kernel Cc: ~postmarketos/upstreaming, phone-devel, Nia Espera Expands sysfs boot_lun attribute to be writable. Necessary to enable proper support for LUN switching on some UFS devices. Signed-off-by: Nia Espera <a5b6@riseup.net> --- 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)) + 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; + + 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [RESEND PATCH] scsi: ufs: sysfs: support writing boot_lun attr 2022-05-25 16:40 [RESEND PATCH] scsi: ufs: sysfs: support writing boot_lun attr Nia Espera @ 2022-05-25 18:34 ` Avri Altman 2022-05-26 20:12 ` - 0 siblings, 1 reply; 10+ messages in thread From: Avri Altman @ 2022-05-25 18:34 UTC (permalink / raw) To: Nia Espera, Alim Akhtar, James E . J . Bottomley, Martin K . Petersen, linux-scsi, linux-kernel Cc: ~postmarketos/upstreaming, phone-devel 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 <a5b6@riseup.net> > --- > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH] scsi: ufs: sysfs: support writing boot_lun attr 2022-05-25 18:34 ` Avri Altman @ 2022-05-26 20:12 ` - 2022-05-27 6:17 ` Avri Altman 0 siblings, 1 reply; 10+ messages in thread From: - @ 2022-05-26 20:12 UTC (permalink / raw) To: Avri Altman, Alim Akhtar, James E . J . Bottomley, Martin K . Petersen, linux-scsi, linux-kernel Cc: ~postmarketos/upstreaming, phone-devel 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 <a5b6@riseup.net> >> --- >> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [RESEND PATCH] scsi: ufs: sysfs: support writing boot_lun attr 2022-05-26 20:12 ` - @ 2022-05-27 6:17 ` Avri Altman 2022-05-27 12:50 ` Caleb Connolly 0 siblings, 1 reply; 10+ messages in thread From: Avri Altman @ 2022-05-27 6:17 UTC (permalink / raw) To: a5b6, Alim Akhtar, James E . J . Bottomley, Martin K . Petersen, linux-scsi, linux-kernel Cc: ~postmarketos/upstreaming, phone-devel > > 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 <a5b6@riseup.net> > >> --- > >> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RE: [RESEND PATCH] scsi: ufs: sysfs: support writing boot_lun attr 2022-05-27 6:17 ` Avri Altman @ 2022-05-27 12:50 ` Caleb Connolly 2022-06-01 17:05 ` Avri Altman 0 siblings, 1 reply; 10+ messages in thread From: Caleb Connolly @ 2022-05-27 12:50 UTC (permalink / raw) To: Avri Altman, a5b6, Alim Akhtar, James E . J . Bottomley, Martin K . Petersen, linux-scsi, linux-kernel Cc: ~postmarketos/upstreaming, phone-devel 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 <a5b6@riseup.net> >>>> --- >>>> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: RE: [RESEND PATCH] scsi: ufs: sysfs: support writing boot_lun attr 2022-05-27 12:50 ` Caleb Connolly @ 2022-06-01 17:05 ` Avri Altman 2022-06-05 3:55 ` Bart Van Assche 0 siblings, 1 reply; 10+ messages in thread From: Avri Altman @ 2022-06-01 17:05 UTC (permalink / raw) To: Caleb Connolly, a5b6, Alim Akhtar, James E . J . Bottomley, Martin K . Petersen, linux-scsi, linux-kernel Cc: ~postmarketos/upstreaming, phone-devel 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@riseup.net> > >>>> --- > >>>> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH] scsi: ufs: sysfs: support writing boot_lun attr 2022-06-01 17:05 ` Avri Altman @ 2022-06-05 3:55 ` Bart Van Assche 2022-06-06 2:48 ` Damien Le Moal 0 siblings, 1 reply; 10+ messages in thread From: Bart Van Assche @ 2022-06-05 3:55 UTC (permalink / raw) To: Avri Altman, Caleb Connolly, a5b6, Alim Akhtar, James E . J . Bottomley, Martin K . Petersen, linux-scsi, linux-kernel Cc: ~postmarketos/upstreaming, phone-devel 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH] scsi: ufs: sysfs: support writing boot_lun attr 2022-06-05 3:55 ` Bart Van Assche @ 2022-06-06 2:48 ` Damien Le Moal 2022-06-06 13:16 ` Bart Van Assche 0 siblings, 1 reply; 10+ messages in thread From: Damien Le Moal @ 2022-06-06 2:48 UTC (permalink / raw) To: Bart Van Assche, Avri Altman, Caleb Connolly, a5b6, Alim Akhtar, James E . J . Bottomley, Martin K . Petersen, linux-scsi, linux-kernel Cc: ~postmarketos/upstreaming, phone-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH] scsi: ufs: sysfs: support writing boot_lun attr 2022-06-06 2:48 ` Damien Le Moal @ 2022-06-06 13:16 ` Bart Van Assche 2022-06-07 0:42 ` Damien Le Moal 0 siblings, 1 reply; 10+ messages in thread From: Bart Van Assche @ 2022-06-06 13:16 UTC (permalink / raw) To: Damien Le Moal, Avri Altman, Caleb Connolly, a5b6, Alim Akhtar, James E . J . Bottomley, Martin K . Petersen, linux-scsi, linux-kernel Cc: ~postmarketos/upstreaming, phone-devel 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH] scsi: ufs: sysfs: support writing boot_lun attr 2022-06-06 13:16 ` Bart Van Assche @ 2022-06-07 0:42 ` Damien Le Moal 0 siblings, 0 replies; 10+ messages in thread From: Damien Le Moal @ 2022-06-07 0:42 UTC (permalink / raw) To: Bart Van Assche, Avri Altman, Caleb Connolly, a5b6, Alim Akhtar, James E . J . Bottomley, Martin K . Petersen, linux-scsi, linux-kernel Cc: ~postmarketos/upstreaming, phone-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-06-07 0:42 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-25 16:40 [RESEND PATCH] scsi: ufs: sysfs: support writing boot_lun attr Nia Espera 2022-05-25 18:34 ` Avri Altman 2022-05-26 20:12 ` - 2022-05-27 6:17 ` Avri Altman 2022-05-27 12:50 ` Caleb Connolly 2022-06-01 17:05 ` Avri Altman 2022-06-05 3:55 ` Bart Van Assche 2022-06-06 2:48 ` Damien Le Moal 2022-06-06 13:16 ` Bart Van Assche 2022-06-07 0:42 ` Damien Le Moal
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.