| Message ID | 1533179408-20631-1-git-send-email-zhongjiang@huawei.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | scsi:NCR5380: remove same check condition in NCR5380_select | expand |
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.
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. > > >
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. >> >> >> > >
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.
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.
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 <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.
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
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);
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(-)