The bsg driver allows user space to send device management commands.
As such, it is often used by field application engineers to debug various problems,
and as a test bed for new features as well.
Let's not bound ourself to hard coded descriptor sizes, as the new
Descriptors that supports new features are not defined yet.
Signed-off-by: Arthur Simchaev <[email protected]>
---
drivers/scsi/ufs/ufs_bsg.c | 28 ++++------------------------
1 file changed, 4 insertions(+), 24 deletions(-)
diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
index 39bf204..7c56eba 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -6,24 +6,6 @@
*/
#include "ufs_bsg.h"
-static int ufs_bsg_get_query_desc_size(struct ufs_hba *hba, int *desc_len,
- struct utp_upiu_query *qr)
-{
- int desc_size = be16_to_cpu(qr->length);
- int desc_id = qr->idn;
-
- if (desc_size <= 0)
- return -EINVAL;
-
- ufshcd_map_desc_id_to_length(hba, desc_id, desc_len);
- if (!*desc_len)
- return -EINVAL;
-
- *desc_len = min_t(int, *desc_len, desc_size);
-
- return 0;
-}
-
static int ufs_bsg_verify_query_size(struct ufs_hba *hba,
unsigned int request_len,
unsigned int reply_len)
@@ -52,13 +34,11 @@ static int ufs_bsg_alloc_desc_buffer(struct ufs_hba *hba, struct bsg_job *job,
goto out;
qr = &bsg_request->upiu_req.qr;
- if (ufs_bsg_get_query_desc_size(hba, desc_len, qr)) {
- dev_err(hba->dev, "Illegal desc size\n");
- return -EINVAL;
- }
+ *desc_len = be16_to_cpu(qr->length);
- if (*desc_len > job->request_payload.payload_len) {
- dev_err(hba->dev, "Illegal desc size\n");
+ if (*desc_len <= 0 || *desc_len > QUERY_DESC_MAX_SIZE ||
+ *desc_len > job->request_payload.payload_len) {
+ dev_err(hba->dev, "Illegal desc size %d\n", *desc_len);
return -EINVAL;
}
--
2.7.4
Hi Martin
The bsg driver allows user space to send device management commands.
As such, it is often used by field application engineers to debug various problems, and as a test bed for new features as well.
Let's not bound ourself to hard coded descriptor sizes, as the new Descriptors that supports new features are not defined yet.
Please consider this patch series for kernel v5.20
Regards
Arthur
> -----Original Message-----
> From: Arthur Simchaev <[email protected]>
> Sent: Monday, June 20, 2022 3:26 PM
> To: James; E.J.Bottomley; [email protected]; Martin; K.Petersen;
> [email protected]
> Cc: [email protected]; [email protected]; Bean; Huo;
> [email protected]; Arthur Simchaev <[email protected]>
> Subject: [PATCH] scsi: ufs-bsg: Remove ufs_bsg_get_query_desc_size function
>
> The bsg driver allows user space to send device management commands.
> As such, it is often used by field application engineers to debug various
> problems,
> and as a test bed for new features as well.
>
> Let's not bound ourself to hard coded descriptor sizes, as the new
> Descriptors that supports new features are not defined yet.
>
> Signed-off-by: Arthur Simchaev <[email protected]>
> ---
> drivers/scsi/ufs/ufs_bsg.c | 28 ++++------------------------
> 1 file changed, 4 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
> index 39bf204..7c56eba 100644
> --- a/drivers/scsi/ufs/ufs_bsg.c
> +++ b/drivers/scsi/ufs/ufs_bsg.c
> @@ -6,24 +6,6 @@
> */
> #include "ufs_bsg.h"
>
> -static int ufs_bsg_get_query_desc_size(struct ufs_hba *hba, int *desc_len,
> - struct utp_upiu_query *qr)
> -{
> - int desc_size = be16_to_cpu(qr->length);
> - int desc_id = qr->idn;
> -
> - if (desc_size <= 0)
> - return -EINVAL;
> -
> - ufshcd_map_desc_id_to_length(hba, desc_id, desc_len);
> - if (!*desc_len)
> - return -EINVAL;
> -
> - *desc_len = min_t(int, *desc_len, desc_size);
> -
> - return 0;
> -}
> -
> static int ufs_bsg_verify_query_size(struct ufs_hba *hba,
> unsigned int request_len,
> unsigned int reply_len)
> @@ -52,13 +34,11 @@ static int ufs_bsg_alloc_desc_buffer(struct ufs_hba
> *hba, struct bsg_job *job,
> goto out;
>
> qr = &bsg_request->upiu_req.qr;
> - if (ufs_bsg_get_query_desc_size(hba, desc_len, qr)) {
> - dev_err(hba->dev, "Illegal desc size\n");
> - return -EINVAL;
> - }
> + *desc_len = be16_to_cpu(qr->length);
>
> - if (*desc_len > job->request_payload.payload_len) {
> - dev_err(hba->dev, "Illegal desc size\n");
> + if (*desc_len <= 0 || *desc_len > QUERY_DESC_MAX_SIZE ||
> + *desc_len > job->request_payload.payload_len) {
> + dev_err(hba->dev, "Illegal desc size %d\n", *desc_len);
> return -EINVAL;
> }
>
> --
> 2.7.4
Arthur,
> The bsg driver allows user space to send device management commands.
> As such, it is often used by field application engineers to debug
> various problems, and as a test bed for new features as well.
I would like a review from UFS stakeholders.
--
Martin K. Petersen Oracle Linux Engineering
On Mon, Jun 20, 2022 at 03:26:06PM +0300, Arthur Simchaev wrote:
> The bsg driver allows user space to send device management commands.
> As such, it is often used by field application engineers to debug various problems,
> and as a test bed for new features as well.
>
> Let's not bound ourself to hard coded descriptor sizes, as the new
> Descriptors that supports new features are not defined yet.
Can you clarify what you mean "hard-coded"? The descriptor size is initialized
as QUERY_DESC_MAX_SIZE, and updated in `ufshcd_update_desc_length`, which is
called with the actual size upon finishing `ufshcd_read_desc_param`.
The function you are removing - `ufs_bsg_get_query_desc_size` - doesn't seem to
reject requests on incompatible size, only to restrict the size of the query.
The way the code is written - if I read it right - will lead to truncation of
the response if the size of the requested response is less than the actual
descriptor size, but otherwise you should get full descriptor back.
Can you provide a specific example where you see the problem to arise?
Thanks,
Daniil
Hi Daniil,
Thanks a lot for your review.
> Can you clarify what you mean "hard-coded"? The descriptor size is initialized
> as QUERY_DESC_MAX_SIZE, and updated in `ufshcd_update_desc_length`,
> which is
> called with the actual size upon finishing `ufshcd_read_desc_param`.
>
> The function you are removing - `ufs_bsg_get_query_desc_size` - doesn't seem
> to
> reject requests on incompatible size, only to restrict the size of the query.
> The way the code is written - if I read it right - will lead to truncation of
> the response if the size of the requested response is less than the actual
> descriptor size, but otherwise you should get full descriptor back.
>
> Can you provide a specific example where you see the problem to arise?
Reading a new UFS descriptors will be rejected due to hard coded descriptor definitions
in ufshcd_map_desc_id_to_length invoked from ufs_bsg_get_query_desc_size.
For example FBO descriptor published in Jedec UFS 4.0 spec and already exist in some UFS devices.
Or others reserved descriptors which can be used as vendor's descriptor.
We should be able to read any UFS descriptor of any size (up to QUERY_DESC_MAX_SIZE).
According to the spec, the device will return the actual size.
The ufs bsg driver should not impose any such limitation. It's one of its design guidelines.
E.g. as done for the attributes, flags the kernel doesn't check it size(for attributes is the max - 4 bytes),
nor access (read/write).
And just returns an appropriate error code should an error occur.
Regards
Arthur
Hi Daniil,
Thanks a lot for your review.
> Can you clarify what you mean "hard-coded"? The descriptor size is initialized
> as QUERY_DESC_MAX_SIZE, and updated in `ufshcd_update_desc_length`,
> which is
> called with the actual size upon finishing `ufshcd_read_desc_param`.
>
> The function you are removing - `ufs_bsg_get_query_desc_size` - doesn't seem
> to
> reject requests on incompatible size, only to restrict the size of the query.
> The way the code is written - if I read it right - will lead to truncation of
> the response if the size of the requested response is less than the actual
> descriptor size, but otherwise you should get full descriptor back.
>
> Can you provide a specific example where you see the problem to arise?
Reading a new UFS descriptors will be rejected due to hard coded descriptor definitions
in ufshcd_map_desc_id_to_length invoked from ufs_bsg_get_query_desc_size.
For example FBO descriptor published in Jedec UFS 4.0 spec and already exist in some UFS devices.
Or others reserved descriptors which can be used as vendor's descriptor.
We should be able to read any UFS descriptor of any size (up to QUERY_DESC_MAX_SIZE).
According to the spec, the device will return the actual size.
The ufs bsg driver should not impose any such limitation. It's one of its design guidelines.
E.g. as done for the attributes, flags the kernel doesn't check it size(for attributes is the max - 4 bytes),
nor access (read/write).
And just returns an appropriate error code should an error occur.
Regards
Arthur
Regards
Arthur
Reviewed-by: Daniil Lunev <[email protected]>
Hi Martin
Please consider applying this patch to the kernel.
Regards
Arthur
> -----Original Message-----
> From: Daniil Lunev <[email protected]>
> Sent: Wednesday, August 24, 2022 12:36 PM
> To: Arthur Simchaev <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Avi Shchislowski
> <[email protected]>
> Subject: Re: [PATCH] scsi: ufs-bsg: Remove ufs_bsg_get_query_desc_size
> function
>
> CAUTION: This email originated from outside of Western Digital. Do not click
> on links or open attachments unless you recognize the sender and know that
> the content is safe.
>
>
> Reviewed-by: Daniil Lunev <[email protected]>
Martin - a kind reminder.
Regards
Arthur
> -----Original Message-----
> From: Arthur Simchaev <[email protected]>
> Sent: Sunday, September 11, 2022 1:35 PM
> To: [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; Avi Shchislowski <[email protected]>; Daniil
> Lunev <[email protected]>
> Subject: RE: [PATCH] scsi: ufs-bsg: Remove ufs_bsg_get_query_desc_size
> function
>
> CAUTION: This email originated from outside of Western Digital. Do not click
> on links or open attachments unless you recognize the sender and know that
> the content is safe.
>
>
> Hi Martin
>
> Please consider applying this patch to the kernel.
>
> Regards
> Arthur
>
> > -----Original Message-----
> > From: Daniil Lunev <[email protected]>
> > Sent: Wednesday, August 24, 2022 12:36 PM
> > To: Arthur Simchaev <[email protected]>
> > Cc: [email protected]; [email protected]; linux-
> > [email protected]; [email protected]; Avi Shchislowski
> > <[email protected]>
> > Subject: Re: [PATCH] scsi: ufs-bsg: Remove ufs_bsg_get_query_desc_size
> > function
> >
> > CAUTION: This email originated from outside of Western Digital. Do not click
> > on links or open attachments unless you recognize the sender and know
> that
> > the content is safe.
> >
> >
> > Reviewed-by: Daniil Lunev <[email protected]>
Arthur,
> Martin - a kind reminder.
I have been waiting for some of the other UFS contributors to chime in.
--
Martin K. Petersen Oracle Linux Engineering
Hi Arthur,
On Mon, 2022-06-20 at 15:26 +0300, Arthur Simchaev wrote:
> The bsg driver allows user space to send device management commands.
> As such, it is often used by field application engineers to debug
> various problems,
> and as a test bed for new features as well.
>
> Let's not bound ourself to hard coded descriptor sizes, as the new
UFS descriptor size is no longer hardcoded (defined in the driver), the
size of the descriptor is reported by UFS itself, check the latest
kernel.
> Descriptors that supports new features are not defined yet.
>
> Signed-off-by: Arthur Simchaev <[email protected]>
> ---
> drivers/scsi/ufs/ufs_bsg.c | 28 ++++------------------------
> 1 file changed, 4 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
This UFS driver is in the wrong location, I assume you are using an
older kernel version?
Kind regards,
Bean
Thank you Bean
> UFS descriptor size is no longer hardcoded (defined in the driver), the
> size of the descriptor is reported by UFS itself, check the latest
> kernel.
>
Invokes ufshcd_map_desc_id_to_length from bsg code, still problematic
also in the latest kernel. The function limited the ufs bsg functionality.
For example FBO descriptor published in Jedec UFS 4.0 spec and already exist in some UFS devices.
Or others reserved descriptors (RFU_0/1) which can be used as vendor's descriptor. The function returns len =0
We should be able to read any UFS descriptor of any size (up to QUERY_DESC_MAX_SIZE) or idn.
According to the spec, the device will return the actual size.
The ufs bsg driver should not impose any such limitation. It's one of its design guidelines.
E.g. as done for the attributes, flags the kernel doesn't check it size(for attributes is the max - 4 bytes),
nor access (read/write).
And just returns an appropriate error code should an error occur.
> This UFS driver is in the wrong location, I assume you are using an
> older kernel version?
Done
Regards
Arthur
> -----Original Message-----
> From: Bean Huo <[email protected]>
> Sent: Tuesday, September 20, 2022 12:38 PM
> To: Arthur Simchaev <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH] scsi: ufs-bsg: Remove ufs_bsg_get_query_desc_size
> function
>
> CAUTION: This email originated from outside of Western Digital. Do not click
> on links or open attachments unless you recognize the sender and know that
> the content is safe.
>
>
> Hi Arthur,
>
>
> On Mon, 2022-06-20 at 15:26 +0300, Arthur Simchaev wrote:
> > The bsg driver allows user space to send device management commands.
> > As such, it is often used by field application engineers to debug
> > various problems,
> > and as a test bed for new features as well.
> >
> > Let's not bound ourself to hard coded descriptor sizes, as the new
>
> UFS descriptor size is no longer hardcoded (defined in the driver), the
> size of the descriptor is reported by UFS itself, check the latest
> kernel.
>
>
> > Descriptors that supports new features are not defined yet.
> >
> > Signed-off-by: Arthur Simchaev <[email protected]>
> > ---
> > drivers/scsi/ufs/ufs_bsg.c | 28 ++++------------------------
> > 1 file changed, 4 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
>
> This UFS driver is in the wrong location, I assume you are using an
> older kernel version?
>
> Kind regards,
> Bean
Hi Bean
In case you don't have any comments I will appreciate if you will add "reviewed by" to the patch.
Regards
Arthur
> -----Original Message-----
> From: Arthur Simchaev
> Sent: Wednesday, September 21, 2022 12:53 PM
> To: Bean Huo <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: RE: [PATCH] scsi: ufs-bsg: Remove ufs_bsg_get_query_desc_size
> function
>
> Thank you Bean
>
> > UFS descriptor size is no longer hardcoded (defined in the driver), the
> > size of the descriptor is reported by UFS itself, check the latest
> > kernel.
> >
> Invokes ufshcd_map_desc_id_to_length from bsg code, still problematic
> also in the latest kernel. The function limited the ufs bsg functionality.
> For example FBO descriptor published in Jedec UFS 4.0 spec and already exist
> in some UFS devices.
> Or others reserved descriptors (RFU_0/1) which can be used as vendor's
> descriptor. The function returns len =0
> We should be able to read any UFS descriptor of any size (up to
> QUERY_DESC_MAX_SIZE) or idn.
> According to the spec, the device will return the actual size.
>
> The ufs bsg driver should not impose any such limitation. It's one of its design
> guidelines.
> E.g. as done for the attributes, flags the kernel doesn't check it size(for
> attributes is the max - 4 bytes),
> nor access (read/write).
> And just returns an appropriate error code should an error occur.
>
> > This UFS driver is in the wrong location, I assume you are using an
> > older kernel version?
> Done
>
> Regards
> Arthur
>
> > -----Original Message-----
> > From: Bean Huo <[email protected]>
> > Sent: Tuesday, September 20, 2022 12:38 PM
> > To: Arthur Simchaev <[email protected]>; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected]
> > Subject: Re: [PATCH] scsi: ufs-bsg: Remove ufs_bsg_get_query_desc_size
> > function
> >
> > CAUTION: This email originated from outside of Western Digital. Do not click
> > on links or open attachments unless you recognize the sender and know
> that
> > the content is safe.
> >
> >
> > Hi Arthur,
> >
> >
> > On Mon, 2022-06-20 at 15:26 +0300, Arthur Simchaev wrote:
> > > The bsg driver allows user space to send device management commands.
> > > As such, it is often used by field application engineers to debug
> > > various problems,
> > > and as a test bed for new features as well.
> > >
> > > Let's not bound ourself to hard coded descriptor sizes, as the new
> >
> > UFS descriptor size is no longer hardcoded (defined in the driver), the
> > size of the descriptor is reported by UFS itself, check the latest
> > kernel.
> >
> >
> > > Descriptors that supports new features are not defined yet.
> > >
> > > Signed-off-by: Arthur Simchaev <[email protected]>
> > > ---
> > > drivers/scsi/ufs/ufs_bsg.c | 28 ++++------------------------
> > > 1 file changed, 4 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
> >
> > This UFS driver is in the wrong location, I assume you are using an
> > older kernel version?
> >
> > Kind regards,
> > Bean
On Wed, 2022-09-28 at 08:33 +0000, Arthur Simchaev wrote:
> Hi Bean
>
> In case you don't have any comments I will appreciate if you will add
> "reviewed by" to the patch.
>
> Regards
> Arthur
Hi Arthur,
I'm thinking we should remove the desc size check in ufshcd.c entirely.
Just read any descriptor with a maximum size of QUERY_DESC_MAX_SIZE .
For user space queries, ufs_bsg reads data of the maximum length and
returns the requested length data. Thus can improve code readability
and save CPU cycles, also can fix your concern.
I don't know how about others' opinion?
Kind regards,
Bean
Hi Bean.
I think in any case we need remove the redundant ufs_bsg_get_query_desc_size
function from ufs_bsg. As done in this patch and submit the another one in order to remove
the desc size check in ufshcd.c entirely.
Are you agree?
Regards
Arthur
> -----Original Message-----
> From: Arthur Simchaev <[email protected]>
> Sent: Wednesday, September 28, 2022 5:42 PM
> To: Bean Huo <[email protected]>; [email protected];
> [email protected]
> Cc: [email protected]; [email protected]; Daniil Lunev
> <[email protected]>; Avri Altman <[email protected]>; Avi
> Shchislowski <[email protected]>
> Subject: RE: [PATCH] scsi: ufs-bsg: Remove ufs_bsg_get_query_desc_size
> function
>
> CAUTION: This email originated from outside of Western Digital. Do not click
> on links or open attachments unless you recognize the sender and know that
> the content is safe.
>
>
> Agree with you. Will change & send the patch.
>
> Regards
> Arthur
>
> > -----Original Message-----
> > From: Bean Huo <[email protected]>
> > Sent: Wednesday, September 28, 2022 1:36 PM
> > To: Arthur Simchaev <[email protected]>;
> > [email protected]; [email protected]
> > Cc: [email protected]; [email protected]; Daniil Lunev
> > <[email protected]>; Avri Altman <[email protected]>; Avi
> > Shchislowski <[email protected]>
> > Subject: Re: [PATCH] scsi: ufs-bsg: Remove ufs_bsg_get_query_desc_size
> > function
> >
> > CAUTION: This email originated from outside of Western Digital. Do not click
> > on links or open attachments unless you recognize the sender and know
> that
> > the content is safe.
> >
> >
> > On Wed, 2022-09-28 at 08:33 +0000, Arthur Simchaev wrote:
> > > Hi Bean
> > >
> > > In case you don't have any comments I will appreciate if you will add
> > > "reviewed by" to the patch.
> > >
> > > Regards
> > > Arthur
> >
> >
> > Hi Arthur,
> >
> > I'm thinking we should remove the desc size check in ufshcd.c entirely.
> > Just read any descriptor with a maximum size of QUERY_DESC_MAX_SIZE .
> > For user space queries, ufs_bsg reads data of the maximum length and
> > returns the requested length data. Thus can improve code readability
> > and save CPU cycles, also can fix your concern.
> >
> > I don't know how about others' opinion?
> >
> > Kind regards,
> > Bean
> >
> >