From: Michael Schmitz <schmitzmic@gmail.com>
To: Finn Thain <fthain@telegraphics.com.au>
Cc: zhong jiang <zhongjiang@huawei.com>,
Bart Van Assche <Bart.VanAssche@wdc.com>,
"jejb@linux.vnet.ibm.com" <jejb@linux.vnet.ibm.com>,
"martin.petersen@oracle.com" <martin.petersen@oracle.com>,
"andy.shevchenko@gmail.com" <andy.shevchenko@gmail.com>,
"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"john.garry@huawei.com" <john.garry@huawei.com>
Subject: Re: [PATCH] scsi:NCR5380: remove same check condition in NCR5380_select
Date: Fri, 3 Aug 2018 16:19:30 +1200 [thread overview]
Message-ID: <6e28be9a-b601-17e2-4a04-9635925e5958@gmail.com> (raw)
In-Reply-To: <alpine.LNX.2.21.1808031148320.16@nippy.intranet>
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
next prev parent reply other threads:[~2018-08-03 4:19 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6e28be9a-b601-17e2-4a04-9635925e5958@gmail.com \
--to=schmitzmic@gmail.com \
--cc=Bart.VanAssche@wdc.com \
--cc=andy.shevchenko@gmail.com \
--cc=fthain@telegraphics.com.au \
--cc=jejb@linux.vnet.ibm.com \
--cc=john.garry@huawei.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=zhongjiang@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).