On Tue, May 19, 2015 at 3:45 PM, Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> wrote:
> On Tue, May 19, 2015 at 02:54:41PM +0800, Daniel Kurtz wrote:
>> >> > + while (1) {
>> >> > + ret = regmap_read(infracfg, INFRA_TOPAXI_PROTECTSTA1, &val);
>> >> > + if (ret)
>> >> > + return ret;
>> >> > +
>> >> > + if ((val & mask) == mask)
>> >> > + break;
>> >> > +
>> >> > + cpu_relax();
>> >> > + if (time_after(jiffies, expired))
>> >> > + return -EIO;
>> >>
>> >> I think we should check for timeout first, and then cpu_relax() if
>> >> there is still time left (here and in
>> >> mtk_infracfg_clear_bus_protection()). Otherwise we end up doing one
>> >> final cpu_relax() without rechecking the register we are polling
>> >> (again, I have the same comment for the timeout loops in mtk-scpsys).
>> >
>> > I think cpu_relax() delays execution in the order of microseconds (I
>> > don't actually know, just a guess), so if the timeout is a second the
>> > order doesn't really matter. What can happen though is an interrupt
>> > after the (val & mask) test but before the timeout check. So to be
>> > truly correct we have to repeat the (val & mask) test after the
>> > time_after() check. Is that what you want?
>>
>> I'm not following, why would you need to repeat (val & mask) test
>> after time_after?
>> What does an interrupt have to do with it?
>> Can you show a code snippet with what exactly you are proposing?
>
> Consider you have this timeout loop:
>
> while (1) {
> if (success())
> break;
>
> if (time_after(jiffies, expired))
> return -ETIMEDOUT;
> }
>
> Now when an interupt comes in between success() and time_after() then it
> can happen that the delay caused by the interrupt makes the code timeout
> even though success() might have become true in the meantime. So to be
> correct you have to:
I agree - I was confused because you only mentioned repeating the
"(val & mask) test", not re-reading the register, which is the
important bit.
For other drivers, I've seen "wait_for()" macros, like below, which do
exactly as you suggest above:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/rockchip-iommu.c#n110
>
> while (1) {
> if (success())
> break;
>
> if (time_after(jiffies, expired)) {
> if (success())
> break;
> return -ETIMEDOUT;
> }
>
> Or, if you don't want to repeat the termination condition:
>
> bool timeout = false;
>
> while (1) {
> if (success())
> break;
>
> if (timeout)
> return -ETIMEDOUT;
>
> if (time_after(jiffies, expired))
> timeout = true;
> }
>
> Anyway, with the timeout of one second used here this is all academic.
I totally agree that this is academic for the loops here and in
SCPSYS, where the timeout is arbitrary and long.
-Dan
>
> Sascha
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
