Re: [PATCH V1] rtc: da9063: access ordering error during RTC interrupt system power on

[Alexandre Belloni <alexandre.belloni@free-electrons.com>]

On 17/12/2015 at 11:37:06 +0000, Opensource [Steve Twiss] wrote :
> On 16 December 2015 23:47 Alexandre Belloni wrote:
> > Subject: Re: [PATCH V1] rtc: da9063: access ordering error during RTC interrupt system power on
> > 
> > This seems mostly fine, however ...
> 
> Hi Alexandre,
> Thanks for reviewing this.
> 
> > On 08/12/2015 at 16:28:39 +0000, Steve Twiss wrote :
> > >  	irq_alarm = platform_get_irq_byname(pdev, "ALARM");
> > >  	ret = devm_request_threaded_irq(&pdev->dev, irq_alarm, NULL,
> > >  					da9063_alarm_event,
> > >  					IRQF_TRIGGER_LOW |
> > IRQF_ONESHOT,
> > >  					"ALARM", rtc);
> > > -	if (ret) {
> > > +	if (ret)
> > >  		dev_err(&pdev->dev, "Failed to request ALARM IRQ %d:
> > %d\n",
> > >  			irq_alarm, ret);
> > > -		return ret;
> > > -	}
> > > -
> > 
> > ... now that requesting the interrupt is optional, you probably want to
> > prevent userspace from thinking it will get an interrupt after setting
> > the alarm by returning -EINVAL in da9063_rtc_read_alarm() and
> > da9063_rtc_set_alarm() in that case.
> > 
> 
> .. I'm not quite certain I understand.
> Does the patch looks worse that it is?
> This part,
> 
> +	if (ret)
> 		dev_err(&pdev->dev, "Failed to request ALARM IRQ %d: %d\n",
> 			irq_alarm, ret);
>  -		return ret;
> 
> looks like it has erased the return ret,
> 
> > 
> > > -	rtc->rtc_dev = devm_rtc_device_register(&pdev->dev,
> > DA9063_DRVNAME_RTC,
> > > -					   &da9063_rtc_ops, THIS_MODULE);
> > > -	if (IS_ERR(rtc->rtc_dev))
> > > -		return PTR_ERR(rtc->rtc_dev);
> > >
> > > -	da9063_data_to_tm(data, &rtc->alarm_time, rtc);
> > > -	rtc->rtc_sync = false;
> > >  	return ret;
> 
> But it does exist at the end of the patch.
> So there will still be an error sent if the IRQ is not requested properly.
> Is this what you meant in your previous e-mail?
> 

Indeed, you are right, I'll apply that patch.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com