On Tue, Jun 19, 2012 at 10:00:36AM +0800, Ming Lei wrote:
> On Tue, Jun 19, 2012 at 6:25 AM, Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> > Have you read Documentation/stable_kernel_patches.txt? Please do so and
>
> I haven't read Documentation/stable_kernel_patches.txt, but read
> Documentation/stable_kernel_rules.txt, :-)
Sorry, you are correct :)
> > see why I can't take this patch for a stable tree. Note that no one has
> > ever reported this as a bug before, and the original poster ran away
> > never to be heard from again, so I really don't think it was a real
> > problem that people ever saw.
>
> If Documentation/stable_kernel_rules.txt is the correct doc for stable rule,
> looks reporter requirement isn't listed in the file, but the below can be found:
>
> - No "theoretical race condition" issues, unless an explanation of
> how the race can be exploited is also provided.
>
> so I marked it as -stable because I have explained how the race can be
> exploited in reality.
Ok, but as this has been there since when, 2.5, I'll refrain from
marking it this way, as no one has reported a real problem like this
before.
> >> >> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxxxxx>
> >> >> ---
> >> >> v2:
> >> >> - take Alan's suggestion to use device_trylock to avoid
> >> >> hanging during shutdown by buggy device or driver
> >> >> - hold parent reference counter
> >> >>
> >> >> drivers/base/core.c | 32 ++++++++++++++++++++++++++++++++
> >> >> 1 file changed, 32 insertions(+)
> >> >>
> >> >> diff --git a/drivers/base/core.c b/drivers/base/core.c
> >> >> index 346be8b..f2fc989 100644
> >> >> --- a/drivers/base/core.c
> >> >> +++ b/drivers/base/core.c
> >> >> @@ -1796,6 +1796,16 @@ out:
> >> >> }
> >> >> EXPORT_SYMBOL_GPL(device_move);
> >> >>
> >> >> +static int __try_lock(struct device *dev)
> >> >> +{
> >> >> + int i = 0;
> >> >> +
> >> >> + while (!device_trylock(dev) && i++ < 100)
> >> >> + msleep(10);
> >> >> +
> >> >> + return i < 100;
> >> >> +}
> >> >
> >> > That's a totally arbritary time, why does this work and other times do
> >> > not? And what is this returning, if the lock was grabbed successfully?
> >>
> >> It is a timeout time and is 1sec now. If the lock can't be held in 1sec, the
> >> function will return 0, otherwise it will return 1 and indicates that the lock
> >> has been held successfully.
> >
> > My point is why 1 second? That's completly arbitrary and means nothing.
>
> 1 second is a estimated value, just like many other timeout values in kernel.
>
> For example, the timeout passed to usb_control_msg() is mostly estimated
> first, then may be adjusted later from some new report.
>
> Another example is one recent xhci fix commit:
>
> 622eb783fe(xHCI: Increase the timeout for controller save/restore state
> operation)
>
> the timeout value is just increased arbitrarily to adapt new device.
>
> I still have more many examples in kernel about timeout value...
Yes, I know this, but now you are putting a limit on the amount of time
a probe function can take, when before, we have never had one. That's
not something to be taken lightly, and is one I know is not true.
> > Why not just do a real lock and try for forever?
>
> IMO, there are two advantages not just doing a real lock for forever:
>
> - avoiding buggy device/driver to hang the system
> - with trylock, we can log the buggy device so that it is a bit
> easier to troubleshoot the buggy drivers, suppose the bug is
> only triggered 1 time in one year or more
No, just fix the driver, I don't want to put a time limit on how long
probe can take, as we never have in the past and I'm sure that whatever
we pick, will be wrong for someone.
I have seen devices that take many seconds, and minutes for some if bad
things happen (i.e. the firmware doesn't download properly). Don't
break people's working systems.
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
