| Message ID | 1655727966-31584-1-git-send-email-Arthur.Simchaev@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: ufs-bsg: Remove ufs_bsg_get_query_desc_size function | expand |
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 <Arthur.Simchaev@wdc.com> > Sent: Monday, June 20, 2022 3:26 PM > To: James; E.J.Bottomley; jejb@linux.vnet.ibm.com; Martin; K.Petersen; > martin.petersen@oracle.com > Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; Bean; Huo; > beanhuo@micron.com; Arthur Simchaev <Arthur.Simchaev@wdc.com> > 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 <Arthur.Simchaev@wdc.com> > --- > 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.
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 <dlunev@chromium.org>
Hi Martin Please consider applying this patch to the kernel. Regards Arthur > -----Original Message----- > From: Daniil Lunev <dlunev@chromium.org> > Sent: Wednesday, August 24, 2022 12:36 PM > To: Arthur Simchaev <Arthur.Simchaev@wdc.com> > Cc: martin.petersen@oracle.com; linux-scsi@vger.kernel.org; linux- > kernel@vger.kernel.org; beanhuo@micron.com; Avi Shchislowski > <Avi.Shchislowski@wdc.com> > 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 <dlunev@chromium.org>
Martin - a kind reminder. Regards Arthur > -----Original Message----- > From: Arthur Simchaev <Arthur.Simchaev@wdc.com> > Sent: Sunday, September 11, 2022 1:35 PM > To: martin.petersen@oracle.com > Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; > beanhuo@micron.com; Avi Shchislowski <Avi.Shchislowski@wdc.com>; Daniil > Lunev <dlunev@chromium.org> > 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 <dlunev@chromium.org> > > Sent: Wednesday, August 24, 2022 12:36 PM > > To: Arthur Simchaev <Arthur.Simchaev@wdc.com> > > Cc: martin.petersen@oracle.com; linux-scsi@vger.kernel.org; linux- > > kernel@vger.kernel.org; beanhuo@micron.com; Avi Shchislowski > > <Avi.Shchislowski@wdc.com> > > 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 <dlunev@chromium.org>
Arthur,
> Martin - a kind reminder.
I have been waiting for some of the other UFS contributors to chime in.
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 <Arthur.Simchaev@wdc.com> > --- > 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 <huobean@gmail.com> > Sent: Tuesday, September 20, 2022 12:38 PM > To: Arthur Simchaev <Arthur.Simchaev@wdc.com>; James@vger.kernel.org; > E.J.Bottomley@vger.kernel.org; jejb@linux.vnet.ibm.com; > Martin@vger.kernel.org; K.Petersen@vger.kernel.org; > martin.petersen@oracle.com > Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; > Bean@vger.kernel.org; Huo@vger.kernel.org; beanhuo@micron.com > 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 <Arthur.Simchaev@wdc.com> > > --- > > 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 <huobean@gmail.com>; James@vger.kernel.org; > E.J.Bottomley@vger.kernel.org; jejb@linux.vnet.ibm.com; > Martin@vger.kernel.org; K.Petersen@vger.kernel.org; > martin.petersen@oracle.com > Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; > Bean@vger.kernel.org; Huo@vger.kernel.org; beanhuo@micron.com > 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 <huobean@gmail.com> > > Sent: Tuesday, September 20, 2022 12:38 PM > > To: Arthur Simchaev <Arthur.Simchaev@wdc.com>; James@vger.kernel.org; > > E.J.Bottomley@vger.kernel.org; jejb@linux.vnet.ibm.com; > > Martin@vger.kernel.org; K.Petersen@vger.kernel.org; > > martin.petersen@oracle.com > > Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; > > Bean@vger.kernel.org; Huo@vger.kernel.org; beanhuo@micron.com > > 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 <Arthur.Simchaev@wdc.com> > > > --- > > > 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
Agree with you. Will change & send the patch. Regards Arthur > -----Original Message----- > From: Bean Huo <huobean@gmail.com> > Sent: Wednesday, September 28, 2022 1:36 PM > To: Arthur Simchaev <Arthur.Simchaev@wdc.com>; > martin.petersen@oracle.com; beanhuo@micron.com > Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; Daniil Lunev > <dlunev@chromium.org>; Avri Altman <Avri.Altman@wdc.com>; Avi > Shchislowski <Avi.Shchislowski@wdc.com> > 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 > >
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 <Arthur.Simchaev@wdc.com> > Sent: Wednesday, September 28, 2022 5:42 PM > To: Bean Huo <huobean@gmail.com>; martin.petersen@oracle.com; > beanhuo@micron.com > Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; Daniil Lunev > <dlunev@chromium.org>; Avri Altman <Avri.Altman@wdc.com>; Avi > Shchislowski <Avi.Shchislowski@wdc.com> > 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 <huobean@gmail.com> > > Sent: Wednesday, September 28, 2022 1:36 PM > > To: Arthur Simchaev <Arthur.Simchaev@wdc.com>; > > martin.petersen@oracle.com; beanhuo@micron.com > > Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; Daniil Lunev > > <dlunev@chromium.org>; Avri Altman <Avri.Altman@wdc.com>; Avi > > Shchislowski <Avi.Shchislowski@wdc.com> > > 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 > > > >
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; }
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 <Arthur.Simchaev@wdc.com> --- drivers/scsi/ufs/ufs_bsg.c | 28 ++++------------------------ 1 file changed, 4 insertions(+), 24 deletions(-)