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

On Tue, 12 Aug 2014 11:01:07 +0900 y@samsung.com 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.

Dear Andrew, 

On 08/23/2014 05:42 AM, Andrew Morton wrote:
> On Tue, 12 Aug 2014 11:01:07 +0900 y@samsung.com 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.

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

Best Regads,
Chanwoo Choi

> 
> --- a/drivers/rtc/rtc-s3c.c~rtc-s3c-define-s3c_rtc-structure-to-remove-global-variables-v2
> +++ a/drivers/rtc/rtc-s3c.c
> @@ -121,6 +121,9 @@ static int s3c_rtc_setaie(struct device
>  	struct s3c_rtc *info = dev_get_drvdata(dev);
>  	unsigned int tmp;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	dev_dbg(info->dev, "%s: aie=%d\n", __func__, enabled);
>  
>  	clk_enable(info->rtc_clk);
> @@ -180,6 +183,9 @@ static int s3c_rtc_gettime(struct device
>  	struct s3c_rtc *info = dev_get_drvdata(dev);
>  	unsigned int have_retried = 0;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	clk_enable(info->rtc_clk);
>   retry_get_time:
>  	rtc_tm->tm_min  = readb(info->base + S3C2410_RTCMIN);
> @@ -224,6 +230,9 @@ static int s3c_rtc_settime(struct device
>  	struct s3c_rtc *info = dev_get_drvdata(dev);
>  	int year = tm->tm_year - 100;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	dev_dbg(dev, "set time %04d.%02d.%02d %02d:%02d:%02d\n",
>  		 1900 + tm->tm_year, tm->tm_mon, tm->tm_mday,
>  		 tm->tm_hour, tm->tm_min, tm->tm_sec);
> @@ -255,6 +264,9 @@ static int s3c_rtc_getalarm(struct devic
>  	struct rtc_time *alm_tm = &alrm->time;
>  	unsigned int alm_en;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	clk_enable(info->rtc_clk);
>  	alm_tm->tm_sec  = readb(info->base + S3C2410_ALMSEC);
>  	alm_tm->tm_min  = readb(info->base + S3C2410_ALMMIN);
> @@ -317,6 +329,9 @@ static int s3c_rtc_setalarm(struct devic
>  	struct rtc_time *tm = &alrm->time;
>  	unsigned int alrm_en;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	clk_enable(info->rtc_clk);
>  	dev_dbg(dev, "s3c_rtc_setalarm: %d, %04d.%02d.%02d %02d:%02d:%02d\n",
>  		 alrm->enabled,
> @@ -357,6 +372,9 @@ static int s3c_rtc_proc(struct device *d
>  	struct s3c_rtc *info = dev_get_drvdata(dev);
>  	unsigned int ticnt;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	clk_enable(info->rtc_clk);
>  	if (info->cpu_type == TYPE_S3C64XX) {
>  		ticnt = readw(info->base + S3C2410_RTCCON);
> @@ -548,7 +566,7 @@ static int s3c_rtc_probe(struct platform
>  		rtc_tm.tm_min	= 0;
>  		rtc_tm.tm_sec	= 0;
>  
> -		s3c_rtc_settime(NULL, &rtc_tm);
> +		s3c_rtc_settime(&pdev->dev, &rtc_tm);
>  
>  		dev_warn(&pdev->dev, "warning: invalid RTC value so initializing it\n");
>  	}
> _
> 
>

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

> Dear Andrew, 
> 
> On 08/23/2014 05:42 AM, Andrew Morton wrote:
> > On Tue, 12 Aug 2014 11:01:07 +0900 y@samsung.com 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.

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@samsung.com> wrote:
> 
>> Dear Andrew, 
>>
>> On 08/23/2014 05:42 AM, Andrew Morton wrote:
>>> On Tue, 12 Aug 2014 11:01:07 +0900 y@samsung.com 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