[PATCH] scsi:NCR5380: remove same check condition in NCR5380_select

[PATCH] scsi:NCR5380: remove same check condition in NCR5380_select

[zhong jiang]

The same check condition is redundant, so remove one of them.

Signed-off-by: zhong jiang <zhongjiang@huawei.com>
---
 drivers/scsi/NCR5380.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 90ea0f5..2ecaf3f 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -999,8 +999,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
 
 	/* Check for lost arbitration */
 	if ((NCR5380_read(INITIATOR_COMMAND_REG) & ICR_ARBITRATION_LOST) ||
-	    (NCR5380_read(CURRENT_SCSI_DATA_REG) & hostdata->id_higher_mask) ||
-	    (NCR5380_read(INITIATOR_COMMAND_REG) & ICR_ARBITRATION_LOST)) {
+	    (NCR5380_read(CURRENT_SCSI_DATA_REG) & hostdata->id_higher_mask)) {
 		NCR5380_write(MODE_REG, MR_BASE);
 		dsprintk(NDEBUG_ARBITRATION, instance, "lost arbitration, deasserting MR_ARBITRATE\n");
 		spin_lock_irq(&hostdata->lock);
-- 
1.7.12.4

Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select

[Bart Van Assche]

On Thu, 2018-08-02 at 11:10 +0800, zhong jiang wrote:
> The same check condition is redundant, so remove one of them.
> 
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> ---
>  drivers/scsi/NCR5380.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
> index 90ea0f5..2ecaf3f 100644
> --- a/drivers/scsi/NCR5380.c
> +++ b/drivers/scsi/NCR5380.c
> @@ -999,8 +999,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
>  
>  	/* Check for lost arbitration */
>  	if ((NCR5380_read(INITIATOR_COMMAND_REG) & ICR_ARBITRATION_LOST) ||
> -	    (NCR5380_read(CURRENT_SCSI_DATA_REG) & hostdata->id_higher_mask) ||
> -	    (NCR5380_read(INITIATOR_COMMAND_REG) & ICR_ARBITRATION_LOST)) {
> +	    (NCR5380_read(CURRENT_SCSI_DATA_REG) & hostdata->id_higher_mask)) {
>  		NCR5380_write(MODE_REG, MR_BASE);
>  		dsprintk(NDEBUG_ARBITRATION, instance, "lost arbitration, deasserting MR_ARBITRATE\n");
>  		spin_lock_irq(&hostdata->lock);

Has this patch been tested?

Thanks,

Bart.

Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select

[zhong jiang]

On 2018/8/2 11:26, Bart Van Assche wrote:
> On Thu, 2018-08-02 at 11:10 +0800, zhong jiang wrote:
>> The same check condition is redundant, so remove one of them.
>>
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>> ---
>>  drivers/scsi/NCR5380.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
>> index 90ea0f5..2ecaf3f 100644
>> --- a/drivers/scsi/NCR5380.c
>> +++ b/drivers/scsi/NCR5380.c
>> @@ -999,8 +999,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
>>  
>>  	/* Check for lost arbitration */
>>  	if ((NCR5380_read(INITIATOR_COMMAND_REG) & ICR_ARBITRATION_LOST) ||
>> -	    (NCR5380_read(CURRENT_SCSI_DATA_REG) & hostdata->id_higher_mask) ||
>> -	    (NCR5380_read(INITIATOR_COMMAND_REG) & ICR_ARBITRATION_LOST)) {
>> +	    (NCR5380_read(CURRENT_SCSI_DATA_REG) & hostdata->id_higher_mask)) {
>>  		NCR5380_write(MODE_REG, MR_BASE);
>>  		dsprintk(NDEBUG_ARBITRATION, instance, "lost arbitration, deasserting MR_ARBITRATE\n");
>>  		spin_lock_irq(&hostdata->lock);
> Has this patch been tested?
 I check the issue by doubletest.cocci. Just review the code by myself. Maybe I miss something else.
 please tell let me know if you any objection.

 Thanks
 zhong jiang
> Thanks,
>
> Bart.
>
>
>

Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select

[Michael Schmitz]

Am 02.08.2018 um 15:45 schrieb zhong jiang:
> On 2018/8/2 11:26, Bart Van Assche wrote:
>> On Thu, 2018-08-02 at 11:10 +0800, zhong jiang wrote:
>>> The same check condition is redundant, so remove one of them.
>>>
>>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>>> ---
>>>  drivers/scsi/NCR5380.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
>>> index 90ea0f5..2ecaf3f 100644
>>> --- a/drivers/scsi/NCR5380.c
>>> +++ b/drivers/scsi/NCR5380.c
>>> @@ -999,8 +999,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
>>>
>>>  	/* Check for lost arbitration */
>>>  	if ((NCR5380_read(INITIATOR_COMMAND_REG) & ICR_ARBITRATION_LOST) ||
>>> -	    (NCR5380_read(CURRENT_SCSI_DATA_REG) & hostdata->id_higher_mask) ||
>>> -	    (NCR5380_read(INITIATOR_COMMAND_REG) & ICR_ARBITRATION_LOST)) {
>>> +	    (NCR5380_read(CURRENT_SCSI_DATA_REG) & hostdata->id_higher_mask)) {
>>>  		NCR5380_write(MODE_REG, MR_BASE);
>>>  		dsprintk(NDEBUG_ARBITRATION, instance, "lost arbitration, deasserting MR_ARBITRATE\n");
>>>  		spin_lock_irq(&hostdata->lock);
>> Has this patch been tested?
>  I check the issue by doubletest.cocci. Just review the code by myself. Maybe I miss something else.
>  please tell let me know if you any objection.

This redundant load of the ICR has been in the driver code for a long 
time. There's a small chance it is intentional, so at least minimal 
testing might be in order.

Finn - does the ICR_ARBITRATION_LOST bit have to be cleared by a write 
to the mode register? In that case, the first load would have been 
redundant and can be omitted without changing driver behaviour?

Cheers,

	Michael


>
>  Thanks
>  zhong jiang
>> Thanks,
>>
>> Bart.
>>
>>
>>
>
>

Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select

[Finn Thain]

On Thu, 2 Aug 2018, Michael Schmitz wrote:

> 
> This redundant load of the ICR has been in the driver code for a long 
> time. There's a small chance it is intentional,

Actually, it is intentional.

> so at least minimal testing might be in order.
> 

Minimal testing is almost useless if you are trying to prove the absence 
of race conditions. SCSI arbitration is a race between targets by design; 
so a race between the CPU and the 5380 is going to be hard to observe.

> Finn - does the ICR_ARBITRATION_LOST bit have to be cleared by a write 
> to the mode register?
> 

Something like that: the write to the mode register does clear the 
ICR_ARBITRATION_LOST bit, because it clears the MR_ARBITRATE bit.

> In that case, the first load would have been redundant and can be 
> omitted without changing driver behaviour?

This code is a faithful rendition of the arbitration flow chart in the 
datasheet, so even if you are right, I wouldn't want to change the code.

Besides, I think your argument assumes that ICR and MR are synchronized, 
and also assumes that targets are obeying the spec.

-- 

> Cheers,
> 
> 	Michael
> 
> 

Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select

[Michael Schmitz]

Hi Finn,

Am 03.08.2018 um 14:56 schrieb Finn Thain:
> On Thu, 2 Aug 2018, Michael Schmitz wrote:
>
>>
>> This redundant load of the ICR has been in the driver code for a long
>> time. There's a small chance it is intentional,
>
> Actually, it is intentional.

I had a hunch it might be ...

>
>> so at least minimal testing might be in order.
>>
>
> Minimal testing is almost useless if you are trying to prove the absence
> of race conditions. SCSI arbitration is a race between targets by design;
> so a race between the CPU and the 5380 is going to be hard to observe.

Agreed - I was clearly being too subtle.

>
>> Finn - does the ICR_ARBITRATION_LOST bit have to be cleared by a write
>> to the mode register?
>>
>
> Something like that: the write to the mode register does clear the
> ICR_ARBITRATION_LOST bit, because it clears the MR_ARBITRATE bit.

Yes, but is that the only way the bit can get cleared? Or could the 
first read see the bit set, and the second read (after checking the bus 
data pattern for a higher arbitrating ID) see it cleared? I.e., is that 
bit latched, or does it just reflect current bus status (same as the 
data register)? (I haven't got the datasheet in front of me, so I'm 
guessing here.)

>> In that case, the first load would have been redundant and can be
>> omitted without changing driver behaviour?
>
> This code is a faithful rendition of the arbitration flow chart in the
> datasheet, so even if you are right, I wouldn't want to change the code.

I think that's a pretty clear hint that the 'arbitration lost' condition 
isn't latched. Anyway, we have no hope to demonstrate by testing that 
this patch (or my suggested alternative) does not change driver 
behaviour. No choice but to leave this as-is.

Cheers,

	Michael

Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select

[Finn Thain]

On Fri, 3 Aug 2018, Michael Schmitz wrote:

> > > Finn - does the ICR_ARBITRATION_LOST bit have to be cleared by a 
> > > write to the mode register?
> > > 
> > 
> > Something like that: the write to the mode register does clear the 
> > ICR_ARBITRATION_LOST bit, because it clears the MR_ARBITRATE bit.
> 
> Yes, but is that the only way the bit can get cleared? [...]

Short of a reset, yes.

> 
> > > In that case, the first load would have been redundant and can be 
> > > omitted without changing driver behaviour?
> > 
> > This code is a faithful rendition of the arbitration flow chart in the 
> > datasheet, so even if you are right, I wouldn't want to change the 
> > code.
> 
> I think that's a pretty clear hint that the 'arbitration lost' condition 
> isn't latched. [...]

It's not a hint. It's just an algorithm with fewer assumptions than the 
one you proposed.

As for latching, the datasheet is pretty clear on that. Writing MR_BASE to 
the mode register clears the ICR_ARBITRATION_LOST bit. As in,

        if ((NCR5380_read(INITIATOR_COMMAND_REG) & ICR_ARBITRATION_LOST) ||
            (NCR5380_read(CURRENT_SCSI_DATA_REG) & hostdata->id_higher_mask) ||
            (NCR5380_read(INITIATOR_COMMAND_REG) & ICR_ARBITRATION_LOST)) {
                NCR5380_write(MODE_REG, MR_BASE);

-- 

Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select

[Finn Thain]

On Thu, 2 Aug 2018, zhong jiang wrote:

> The same check condition is redundant, so remove one of them.
> 

If you are trying to find redundant code, your coccinelle script is 
dangerously flawed.

You need to realize that NCR5380_read(CURRENT_SCSI_DATA_REG) is not simply 
a value in a CPU register or a device register, but it is the actual state 
of the scsi bus data lines, which are subject to change any time, at the 
whim of the firmwares in all of the SCSI bus devices participating in 
arbitration at the time, or of a user who might kick a plug etc.

From the datasheet:

    The SCSI data lines are not actually registered, but gated directly 
    onto the CPU bus whenever Address 000 is read by the CPU...

Hence, NAK.

-- 

> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> ---
>  drivers/scsi/NCR5380.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
> index 90ea0f5..2ecaf3f 100644
> --- a/drivers/scsi/NCR5380.c
> +++ b/drivers/scsi/NCR5380.c
> @@ -999,8 +999,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
>  
>  	/* Check for lost arbitration */
>  	if ((NCR5380_read(INITIATOR_COMMAND_REG) & ICR_ARBITRATION_LOST) ||
> -	    (NCR5380_read(CURRENT_SCSI_DATA_REG) & hostdata->id_higher_mask) ||
> -	    (NCR5380_read(INITIATOR_COMMAND_REG) & ICR_ARBITRATION_LOST)) {
> +	    (NCR5380_read(CURRENT_SCSI_DATA_REG) & hostdata->id_higher_mask)) {
>  		NCR5380_write(MODE_REG, MR_BASE);
>  		dsprintk(NDEBUG_ARBITRATION, instance, "lost arbitration, deasserting MR_ARBITRATE\n");
>  		spin_lock_irq(&hostdata->lock);
> 

Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select

[Andy Shevchenko]

On Fri, Aug 3, 2018 at 5:24 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
> On Thu, 2 Aug 2018, zhong jiang wrote:
>
>> The same check condition is redundant, so remove one of them.
>>
>
> If you are trying to find redundant code, your coccinelle script is
> dangerously flawed.

These days too many coccinelle helpers make people think they are
doing right "clean ups" when in the practice they bring the
regressions.

Julia, is possible by coccinelle to distinguish memory accesses versus
I/O? At least it would increase robustness in some cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select

[Julia Lawall]

On Fri, 3 Aug 2018, Andy Shevchenko wrote:

> On Fri, Aug 3, 2018 at 5:24 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
> > On Thu, 2 Aug 2018, zhong jiang wrote:
> >
> >> The same check condition is redundant, so remove one of them.
> >>
> >
> > If you are trying to find redundant code, your coccinelle script is
> > dangerously flawed.
>
> These days too many coccinelle helpers make people think they are
> doing right "clean ups" when in the practice they bring the
> regressions.
>
> Julia, is possible by coccinelle to distinguish memory accesses versus
> I/O? At least it would increase robustness in some cases.

With make coccicheck, the semantic patch should already emit the warning:

//# A common source of false positives is when the argument performs a side
//# effect.

I can modify the rule so that it doesn't report on code that involves
function calls.  It could lose some desirable warnings, where the function
call is just a wrapper for eg extracting some field, but it is probably
safer in practice.

julia