From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755630AbbLQLpy (ORCPT ); Thu, 17 Dec 2015 06:45:54 -0500 Received: from down.free-electrons.com ([37.187.137.238]:60728 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751664AbbLQLpx (ORCPT ); Thu, 17 Dec 2015 06:45:53 -0500 Date: Thu, 17 Dec 2015 12:44:39 +0100 From: Alexandre Belloni To: "Opensource [Steve Twiss]" Cc: Alessandro Zummo , LINUXKERNEL , RTC-LINUX , David Dajun Chen , Support Opensource Subject: Re: [PATCH V1] rtc: da9063: access ordering error during RTC interrupt system power on Message-ID: <20151217114439.GF13078@piout.net> References: <20151208163406.B34683FB25@swsrvapps-01.diasemi.com> <20151216234702.GK8574@piout.net> <6ED8E3B22081A4459DAC7699F3695FB70173D28DCB@SW-EX-MBX02.diasemi.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6ED8E3B22081A4459DAC7699F3695FB70173D28DCB@SW-EX-MBX02.diasemi.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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