Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Avri Altman <Avri.Altman@wdc.com>
To:     Nia Espera <a5b6@riseup.net>,
        Alim Akhtar <alim.akhtar@samsung.com>,
        "James E . J . Bottomley" <jejb@linux.ibm.com>,
        "Martin K . Petersen" <martin.petersen@oracle.com>,
        "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     "~postmarketos/upstreaming@lists.sr.ht" 
        <~postmarketos/upstreaming@lists.sr.ht>,
        "phone-devel@vger.kernel.org" <phone-devel@vger.kernel.org>
Subject: RE: [RESEND PATCH] scsi: ufs: sysfs: support writing boot_lun attr
Thread-Topic: [RESEND PATCH] scsi: ufs: sysfs: support writing boot_lun attr
Thread-Index: AQHYcFYp99UCK0BeAUyoAx4mv1ya/K0v5+fA
Date:   Wed, 25 May 2022 18:34:34 +0000
Message-ID: <DM6PR04MB65750969ACD36EEEB48374DFFCD69@DM6PR04MB6575.namprd04.prod.outlook.com>
References: <20220525164013.93748-1-a5b6@riseup.net>
In-Reply-To: <20220525164013.93748-1-a5b6@riseup.net>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?x4qg73AVIj7eOUW6w1ZyOmHa6PVLCvKlle17s4eznkDFdfWC3MYRM0OJSIiv?=
 =?us-ascii?Q?pu6R5pwiejRLIpbotEHy1rEBFeq5hUuj7CEzQ3H/GeeHLL+g47vXHzYPoiqd?=
 =?us-ascii?Q?MrY6pg43VjihvTI9LBHl2sUAWaTyF9YDDG1ZHOjgCLgw6COwyPpd/k9nwxdb?=
 =?us-ascii?Q?qiTkTunBORJ5ruoY37vLN1JEuX7BuSCsHsQ3mOx8eft7KhN2kDPEYFCiUGyC?=
 =?us-ascii?Q?2Hd2v7SeZqmky6nqjAA9Tzot6a74mHTq/8XYESI07zzMOUJWi72ixTKkbWbQ?=
 =?us-ascii?Q?MdoIOlX59I2uVIaOpA5Uu+3ooZquRjaXSrWb60/E7R8WNl+y75cPoJ5n0XdZ?=
 =?us-ascii?Q?FPuuFXJ9ReXOJ0SenbqyQwdH3fk4w8+KEwtM9SfEJ8bo0GOeaFFBjUyYrVQB?=
 =?us-ascii?Q?NoSJ4zSdajlijug7kf4MO9ECE+tOb1SXv4LTojXE73PfmQYPW1WNRP8K4Jym?=
 =?us-ascii?Q?73hpIS+WDNV6+VNfpWPO4x0Axjq7Uaf8tUg3L/02P7pDgwRDwefcRbEjpjxr?=
 =?us-ascii?Q?BuQoKwzMqsXbocbP4Dg7+08j/G8XkPP1TwI/7qFmJR8KnykHEAZPKGuuQET+?=
 =?us-ascii?Q?8BVhimRIg1Ej4Wevj88EXDtYpdGGwsQ8xjNk7Hq0JjCGE7PqLzA2Rsydw9BO?=
 =?us-ascii?Q?0K++qSH5JdTuRUXX1AaEyikkJ53BqDSxKeLz3jWKnjU+ruadvsoWmCKbKPGQ?=
 =?us-ascii?Q?udtswGTh6olsTvH49kRMMJP8hFubHfsE9dvO3T/v/mRCIWI+tPG9uThtfjP8?=
 =?us-ascii?Q?WXyb0T5esT1QE3DUF8moPAxsynDH2cdiZiB8AfXvdBsskHEt9yaqKUMQq9+/?=
 =?us-ascii?Q?fid75i46AVxz0p1mqBsIZlTBUSKg3V/JXnXbC6D49zuWWdAt38swB6TNdk6c?=
 =?us-ascii?Q?Tk/BlsuCsDPvSoRJ/LnI7WDJkUlEIMI+t6xEp0j6/dxt2RuN38FlPcRVtAFd?=
 =?us-ascii?Q?OZE9CcxUMNPAZEjeCcUqZdtXA+YfkwqyA6CcIpIO+1LTdz6sdV53IwIYBZCP?=
 =?us-ascii?Q?X4GHOBsqCFZaBtm1ISA9epvRxNQPilBOKkeRC1nE+zGnq6V5WReH7BJ4YH+Q?=
 =?us-ascii?Q?SHxyDKWw5/U8k/pKdKBT0CjAln7wTTjfDybtDoHBa8+pZvrRXQk+k2SQBRFL?=
 =?us-ascii?Q?IUA2dwz3jdxClPCpJcBQREXeBE/z+UGuIKmxjJC7JIGTi2ALlbAUgNpdxu91?=
 =?us-ascii?Q?SVaL7k4KY4gjMeVV5IgaafCotS8X7QPOkLAVrG2kQTLQwHlJVoBqIx5clHN1?=
 =?us-ascii?Q?gYjXAIivFN9K6Yg8bcgk4GWu/7RljbmUPMHi/76pQPN7e02wxInJFOK77S0w?=
 =?us-ascii?Q?iUpj76ILCM24N9quu5tkR2HhNvUxICAuKzc8WfbZE3AoGRFz+mKfOzePs1jP?=
 =?us-ascii?Q?aMAepmWRYXbgI8dwiq+G8dx+YuK9nn7pnjWAlBLB+fBaLU2/H2YAl8eCsRpP?=
 =?us-ascii?Q?ZJiNz6KyfjeKnJE13BlRRCQutQ+IlGWxS8kGwk0yZqpeBiozFJOrupZBj7Ej?=
 =?us-ascii?Q?XdrZPRzX7lXchpGqudd67WH6WXB+xlRb50eVicJGx+nv/w0KoISMZDAofBhR?=
 =?us-ascii?Q?gFtaJwODxBThN1kuHQgVWBJj5o4xazMaHj6OACVzETLK80TolpqICSBKY7kw?=
 =?us-ascii?Q?fR+lEtjEFlbx1K4ZPhB2I5H2F4nPxU3iiDjsNLc8vwTi7EePfZkCizhGYAoY?=
 =?us-ascii?Q?jHOmkeGIrbWLpMCuIuSdqI67WcqBu5beJo7cT1xjhDBTFtB/4izdTjvPbeSi?=
 =?us-ascii?Q?O/o1ZpMIwA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: wdc.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR04MB6575.namprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 59446934-fa2e-41b3-19e5-08da3e7d3729
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 May 2022 18:34:34.2995
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: FuaCDS4Cx+bEAQ+Mojrckeq+pTGH6mfksbmXRy7AgYYpJqsUwyz48XQBWsy+E4b5xN2J++sGzYTdh0iO3/bFuQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR04MB7098
X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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?

>=20
> Signed-off-by: Nia Espera <a5b6@riseup.net>
> ---
>  drivers/scsi/ufs/ufs-sysfs.c | 67 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 1 deletion(-)
>=20
> 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_id=
n
> idn)
>                 idn <=3D QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE;
>  }
>=20
> +static ssize_t boot_lun_enabled_show(struct device *dev,
> +                                    struct device_attribute *attr, char =
*buf)
> +{
> +       struct ufs_hba *hba =3D dev_get_drvdata(dev);
> +       u32 slot;
> +       int ret;
> +       u8 index =3D 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 =3D ufshcd_wb_get_query_index(hba);
> +       ufshcd_rpm_get_sync(hba);
> +
> +       ret =3D 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 =3D -EINVAL;
> +               goto out;
> +       }
> +
> +       ret =3D 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 =3D dev_get_drvdata(dev);
> +       u32 slot;
> +       int ret;
> +       u8 index =3D 0;
> +
> +       if (kstrtouint(buf, 0, &slot) < 0)
> +               return -EINVAL;
You need to verify that no one set bBootLunEn =3D 0x0 because the device wo=
n't boot.
Better check explicitly that slot !=3D 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 =3D ufshcd_wb_get_query_index(hba);
> +       ufshcd_rpm_get_sync(hba);
> +
> +       ret =3D 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 =3D -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);
>=20
> -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