[6/8] scsi: ufs: Make ufshcd_poll() complain about unsupported arguments

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(+)

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