Re: [PATCHv2 1/5] rtc: s3c: Define s3c_rtc structure to remove global variables.

[Andrew Morton]

On Mon, 25 Aug 2014 09:57:33 +0900 Chanwoo Choi <cw00.choi@xxxxxxxxxxx> wrote:

> Dear Andrew, 
> 
> On 08/23/2014 05:42 AM, Andrew Morton wrote:
> > On Tue, 12 Aug 2014 11:01:07 +0900 y@xxxxxxxxxxx wrote:
> > 
> >> This patch define s3c_rtc structure including necessary variables for S3C RTC
> >> device instead of global variables. This patch improves the readability by
> >> removing global variables.
> > 
> > Below is the v1->v2 delta.
> > 
> > Why were all those tests of info->base added?  Can it really be zero? 
> > I don't see how.
> 
> If some functions (e.g., s3c_rtc_settime) accesses the rtc register
> by using info->base before the initialization of info->base in s3c_rtc_probe,
> I thought that null pointer error would happen.

probe() should be called before anything else.  If we're somehow
calling s3c_rtc_settime() before probe() has completed then something
very bad is happening - for example, the device may have been
registered far too early.  But I don't think that's the case here.

That being said, it does seem strange that s3c_rtc_probe() calls
devm_rtc_device_register() *before* trying to request its IRQs.  So if
IRQ requesting fails, we go and immediately unregister the device. 
Some other drivers do it this way, others do not.  Wouldn't it be
better to defer registration until we know that all the probe() setup
operations have succeeded?

> But, I missed one point which info->base might have the garbate data instead of NULL.
> I'll add the initialization code for info->base.
> 	info->base = NULL;
> 
> If you don't agree it, I'll drop this code checking the state of info->base on next patchset(v3).

Well, we should have those checks in there unless we know they're
needed.  And if they *are* needed, we should have a good understanding
of why they're needed, and we should be sure that we're not just
working around some underlying problem.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/