* [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select
@ 2018-08-02 3:10 zhong jiang
2018-08-02 3:26 ` Bart Van Assche
2018-08-03 2:24 ` Finn Thain
0 siblings, 2 replies; 10+ messages in thread
From: zhong jiang @ 2018-08-02 3:10 UTC (permalink / raw)
To: fthain, schmitzmic, jejb, martin.petersen
Cc: andy.shevchenko, john.garry, linux-scsi, linux-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select
2018-08-02 3:10 [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select zhong jiang
@ 2018-08-02 3:26 ` Bart Van Assche
2018-08-02 3:45 ` zhong jiang
2018-08-03 2:24 ` Finn Thain
1 sibling, 1 reply; 10+ messages in thread
From: Bart Van Assche @ 2018-08-02 3:26 UTC (permalink / raw)
To: jejb, zhongjiang, martin.petersen, fthain, schmitzmic
Cc: andy.shevchenko, linux-scsi, linux-kernel, john.garry
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select
2018-08-02 3:26 ` Bart Van Assche
@ 2018-08-02 3:45 ` zhong jiang
2018-08-02 7:32 ` Michael Schmitz
0 siblings, 1 reply; 10+ messages in thread
From: zhong jiang @ 2018-08-02 3:45 UTC (permalink / raw)
To: Bart Van Assche
Cc: jejb, martin.petersen, fthain, schmitzmic, andy.shevchenko,
linux-scsi, linux-kernel, john.garry
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.
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select
2018-08-02 3:45 ` zhong jiang
@ 2018-08-02 7:32 ` Michael Schmitz
2018-08-03 2:56 ` Finn Thain
0 siblings, 1 reply; 10+ messages in thread
From: Michael Schmitz @ 2018-08-02 7:32 UTC (permalink / raw)
To: zhong jiang, Bart Van Assche
Cc: jejb, martin.petersen, fthain, andy.shevchenko, linux-scsi,
linux-kernel, john.garry
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.
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select
2018-08-02 3:10 [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select zhong jiang
2018-08-02 3:26 ` Bart Van Assche
@ 2018-08-03 2:24 ` Finn Thain
2018-08-03 9:10 ` Andy Shevchenko
1 sibling, 1 reply; 10+ messages in thread
From: Finn Thain @ 2018-08-03 2:24 UTC (permalink / raw)
To: zhong jiang
Cc: schmitzmic, jejb, martin.petersen, andy.shevchenko, john.garry,
linux-scsi, linux-kernel
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);
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select
2018-08-02 7:32 ` Michael Schmitz
@ 2018-08-03 2:56 ` Finn Thain
2018-08-03 4:19 ` Michael Schmitz
0 siblings, 1 reply; 10+ messages in thread
From: Finn Thain @ 2018-08-03 2:56 UTC (permalink / raw)
To: Michael Schmitz
Cc: zhong jiang, Bart Van Assche, jejb, martin.petersen,
andy.shevchenko, linux-scsi, linux-kernel, john.garry
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
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select
2018-08-03 2:56 ` Finn Thain
@ 2018-08-03 4:19 ` Michael Schmitz
2018-08-03 6:04 ` Finn Thain
0 siblings, 1 reply; 10+ messages in thread
From: Michael Schmitz @ 2018-08-03 4:19 UTC (permalink / raw)
To: Finn Thain
Cc: zhong jiang, Bart Van Assche, jejb, martin.petersen,
andy.shevchenko, linux-scsi, linux-kernel, john.garry
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select
2018-08-03 4:19 ` Michael Schmitz
@ 2018-08-03 6:04 ` Finn Thain
0 siblings, 0 replies; 10+ messages in thread
From: Finn Thain @ 2018-08-03 6:04 UTC (permalink / raw)
To: Michael Schmitz
Cc: zhong jiang, Bart Van Assche, jejb, martin.petersen,
andy.shevchenko, linux-scsi, linux-kernel, john.garry
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);
--
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select
2018-08-03 2:24 ` Finn Thain
@ 2018-08-03 9:10 ` Andy Shevchenko
2018-08-03 9:52 ` Julia Lawall
0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2018-08-03 9:10 UTC (permalink / raw)
To: Finn Thain, Julia Lawall
Cc: zhong jiang, Michael Schmitz, James E . J . Bottomley,
Martin K. Petersen, John Garry, linux-scsi,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select
2018-08-03 9:10 ` Andy Shevchenko
@ 2018-08-03 9:52 ` Julia Lawall
0 siblings, 0 replies; 10+ messages in thread
From: Julia Lawall @ 2018-08-03 9:52 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Finn Thain, zhong jiang, Michael Schmitz,
James E . J . Bottomley, Martin K. Petersen, John Garry,
linux-scsi, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-08-03 9:52 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-02 3:10 [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select zhong jiang
2018-08-02 3:26 ` Bart Van Assche
2018-08-02 3:45 ` zhong jiang
2018-08-02 7:32 ` Michael Schmitz
2018-08-03 2:56 ` Finn Thain
2018-08-03 4:19 ` Michael Schmitz
2018-08-03 6:04 ` Finn Thain
2018-08-03 2:24 ` Finn Thain
2018-08-03 9:10 ` Andy Shevchenko
2018-08-03 9:52 ` Julia Lawall
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).