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

[Chanwoo Choi]

Dear Andrew,

On 08/27/2014 06:31 AM, Andrew Morton wrote:
> 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.

I means that existing rtc-s3c.c driver executed s3c_rtc_settime() in s3c_rtc_probe()
before initialization of info->base. But, It is my mistake. so, I modified it just as following:

-		s3c_rtc_settime(NULL, &rtc_tm);
+		s3c_rtc_settime(&pdev->dev, &rtc_tm);

> 
> 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?

You're right. I missed this point. If rtc-s3c.c driver completed the probe function,
info->base has always right address.

+	if (!info->base)
+		return -EINVAL;
+

As you said, checking state of 'info-base' is un-needed.
I'll send new patchset(v3) to fix it.

> 
>> 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.

You are right. Thanks for your comment and advice.

Best Regards,
Chanwoo Choi

--
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/