The same check condition is redundant, so remove one of them.
Signed-off-by: zhong jiang <[email protected]>
---
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
On Thu, 2018-08-02 at 11:10 +-0800, zhong jiang wrote:
+AD4- The same check condition is redundant, so remove one of them.
+AD4-
+AD4- Signed-off-by: zhong jiang +ADw-zhongjiang+AEA-huawei.com+AD4-
+AD4- ---
+AD4- drivers/scsi/NCR5380.c +AHw- 3 +---
+AD4- 1 file changed, 1 insertion(+-), 2 deletions(-)
+AD4-
+AD4- diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
+AD4- index 90ea0f5..2ecaf3f 100644
+AD4- --- a/drivers/scsi/NCR5380.c
+AD4- +-+-+- b/drivers/scsi/NCR5380.c
+AD4- +AEAAQA- -999,8 +-999,7 +AEAAQA- static struct scsi+AF8-cmnd +ACo-NCR5380+AF8-select(struct Scsi+AF8-Host +ACo-instance,
+AD4-
+AD4- /+ACo- Check for lost arbitration +ACo-/
+AD4- if ((NCR5380+AF8-read(INITIATOR+AF8-COMMAND+AF8-REG) +ACY- ICR+AF8-ARBITRATION+AF8-LOST) +AHwAfA-
+AD4- - (NCR5380+AF8-read(CURRENT+AF8-SCSI+AF8-DATA+AF8-REG) +ACY- hostdata-+AD4-id+AF8-higher+AF8-mask) +AHwAfA-
+AD4- - (NCR5380+AF8-read(INITIATOR+AF8-COMMAND+AF8-REG) +ACY- ICR+AF8-ARBITRATION+AF8-LOST)) +AHs-
+AD4- +- (NCR5380+AF8-read(CURRENT+AF8-SCSI+AF8-DATA+AF8-REG) +ACY- hostdata-+AD4-id+AF8-higher+AF8-mask)) +AHs-
+AD4- NCR5380+AF8-write(MODE+AF8-REG, MR+AF8-BASE)+ADs-
+AD4- dsprintk(NDEBUG+AF8-ARBITRATION, instance, +ACI-lost arbitration, deasserting MR+AF8-ARBITRATE+AFw-n+ACI-)+ADs-
+AD4- spin+AF8-lock+AF8-irq(+ACY-hostdata-+AD4-lock)+ADs-
Has this patch been tested?
Thanks,
Bart.
On 2018/8/2 11:26, Bart Van Assche wrote:
> On Thu, 2018-08-02 at 11:10 +ACs-0800, zhong jiang wrote:
>> The same check condition is redundant, so remove one of them.
>>
>> Signed-off-by: zhong jiang <zhongjiang+AEA-huawei.com>
>> ---
>> drivers/scsi/NCR5380.c +AHw- 3 +ACs---
>> 1 file changed, 1 insertion(+ACs-), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
>> index 90ea0f5..2ecaf3f 100644
>> --- a/drivers/scsi/NCR5380.c
>> +ACsAKwAr- b/drivers/scsi/NCR5380.c
>> +AEAAQA- -999,8 +ACs-999,7 +AEAAQA- static struct scsi+AF8-cmnd +ACo-NCR5380+AF8-select(struct Scsi+AF8-Host +ACo-instance,
>>
>> /+ACo- Check for lost arbitration +ACo-/
>> if ((NCR5380+AF8-read(INITIATOR+AF8-COMMAND+AF8-REG) +ACY- ICR+AF8-ARBITRATION+AF8-LOST) +AHwAfA-
>> - (NCR5380+AF8-read(CURRENT+AF8-SCSI+AF8-DATA+AF8-REG) +ACY- hostdata->id+AF8-higher+AF8-mask) +AHwAfA-
>> - (NCR5380+AF8-read(INITIATOR+AF8-COMMAND+AF8-REG) +ACY- ICR+AF8-ARBITRATION+AF8-LOST)) +AHs-
>> +ACs- (NCR5380+AF8-read(CURRENT+AF8-SCSI+AF8-DATA+AF8-REG) +ACY- hostdata->id+AF8-higher+AF8-mask)) +AHs-
>> NCR5380+AF8-write(MODE+AF8-REG, MR+AF8-BASE);
>> dsprintk(NDEBUG+AF8-ARBITRATION, instance, "lost arbitration, deasserting MR+AF8-ARBITRATE+AFw-n");
>> spin+AF8-lock+AF8-irq(+ACY-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.
>
>
>
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 <[email protected]>
>>> ---
>>> 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.
>>
>>
>>
>
>
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 <[email protected]>
> ---
> 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);
>
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
>
>
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
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);
--
On Fri, Aug 3, 2018 at 5:24 AM, Finn Thain <[email protected]> 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
On Fri, 3 Aug 2018, Andy Shevchenko wrote:
> On Fri, Aug 3, 2018 at 5:24 AM, Finn Thain <[email protected]> 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