| Message ID | 20240617210844.337476-7-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | UFS patches for kernel 6.11 | expand |
On Mon, Jun 17, 2024 at 02:07:45PM -0700, Bart Van Assche wrote: > The ufshcd_poll() implementation does not support queue_num == > UFSHCD_POLL_FROM_INTERRUPT_CONTEXT in MCQ mode. Hence complain > if queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT in MCQ mode. > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/ufs/core/ufshcd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 7761ccca2115..db374a788140 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -5562,6 +5562,7 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num) > struct ufs_hw_queue *hwq; > > if (is_mcq_enabled(hba)) { > + WARN_ON_ONCE(queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT); So what does the user has to do if this WARN_ON gets triggered? Can't we use dev_err()/dev_warn() and return instead if the intention is to just report the error or warning. I know that UFS code has WARN_ON sprinkled all over, but those should be audited at some point and also the new additions. Also, this is a bug fix as it essentially fixes array out of the bounds issue. So should have a fixes tag and CC stable list for backporting. - Mani > hwq = &hba->uhq[queue_num]; > > return ufshcd_mcq_poll_cqe_lock(hba, hwq);
On 6/19/24 12:32 AM, Manivannan Sadhasivam wrote: > On Mon, Jun 17, 2024 at 02:07:45PM -0700, Bart Van Assche wrote: >> The ufshcd_poll() implementation does not support queue_num == >> UFSHCD_POLL_FROM_INTERRUPT_CONTEXT in MCQ mode. Hence complain >> if queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT in MCQ mode. >> >> Signed-off-by: Bart Van Assche <bvanassche@acm.org> >> --- >> drivers/ufs/core/ufshcd.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c >> index 7761ccca2115..db374a788140 100644 >> --- a/drivers/ufs/core/ufshcd.c >> +++ b/drivers/ufs/core/ufshcd.c >> @@ -5562,6 +5562,7 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num) >> struct ufs_hw_queue *hwq; >> >> if (is_mcq_enabled(hba)) { >> + WARN_ON_ONCE(queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT); > > So what does the user has to do if this WARN_ON gets triggered? Can't we use > dev_err()/dev_warn() and return instead if the intention is to just report the > error or warning. > > I know that UFS code has WARN_ON sprinkled all over, but those should be audited > at some point and also the new additions. > > Also, this is a bug fix as it essentially fixes array out of the bounds issue. > So should have a fixes tag and CC stable list for backporting. No, this is not a bug fix. There is only one caller that passes the value UFSHCD_POLL_FROM_INTERRUPT_CONTEXT as the 'queue_num' argument and it is a code path that supports legacy mode (single queue mode). Since the above WARN_ON_ONCE() is in an MCQ code path, it will never be triggered. The above WARN_ON_ONCE() can be seen as a form of documentation and also as defensive programming. I think using WARN_ON_ONCE() to document which code paths will never be triggered is fine. Thanks, Bart.
On Thu, Jun 20, 2024 at 01:13:10PM -0700, Bart Van Assche wrote: > On 6/19/24 12:32 AM, Manivannan Sadhasivam wrote: > > On Mon, Jun 17, 2024 at 02:07:45PM -0700, Bart Van Assche wrote: > > > The ufshcd_poll() implementation does not support queue_num == > > > UFSHCD_POLL_FROM_INTERRUPT_CONTEXT in MCQ mode. Hence complain > > > if queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT in MCQ mode. > > > > > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > > > --- > > > drivers/ufs/core/ufshcd.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > > > index 7761ccca2115..db374a788140 100644 > > > --- a/drivers/ufs/core/ufshcd.c > > > +++ b/drivers/ufs/core/ufshcd.c > > > @@ -5562,6 +5562,7 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num) > > > struct ufs_hw_queue *hwq; > > > if (is_mcq_enabled(hba)) { > > > + WARN_ON_ONCE(queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT); > > > > So what does the user has to do if this WARN_ON gets triggered? Can't we use > > dev_err()/dev_warn() and return instead if the intention is to just report the > > error or warning. > > > > I know that UFS code has WARN_ON sprinkled all over, but those should be audited > > at some point and also the new additions. > > > > Also, this is a bug fix as it essentially fixes array out of the bounds issue. > > So should have a fixes tag and CC stable list for backporting. > > No, this is not a bug fix. There is only one caller that passes the value > UFSHCD_POLL_FROM_INTERRUPT_CONTEXT as the 'queue_num' argument and it is a code > path that supports legacy mode (single queue mode). Since the above WARN_ON_ONCE() > is in an MCQ code path, it will never be triggered. The above WARN_ON_ONCE() can > be seen as a form of documentation and also as defensive programming. I think > using WARN_ON_ONCE() to document which code paths will never be triggered is fine. > Why should we insert a warning in a dead code? WARN_ON* makes sense if a certain condition is never expected to happen, but if that happens then most likely something wrong happened seriously so the users should be warned. But here I don't see a possibility to get this triggered at all. Please correct me if I'm wrong. - Mani
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 7761ccca2115..db374a788140 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -5562,6 +5562,7 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num) struct ufs_hw_queue *hwq; if (is_mcq_enabled(hba)) { + WARN_ON_ONCE(queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT); hwq = &hba->uhq[queue_num]; return ufshcd_mcq_poll_cqe_lock(hba, hwq);
The ufshcd_poll() implementation does not support queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT in MCQ mode. Hence complain if queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT in MCQ mode. Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/ufs/core/ufshcd.c | 1 + 1 file changed, 1 insertion(+)