linux-kernel - RE: [PATCH v3 1/2] rtc: add new lp8788 rtc driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 21 Dec 2012 16:09:30 +0000
From:	"Kim, Milo" <Milo.Kim@...com>
To:	"devendra.aaru" <devendra.aaru@...il.com>
CC:	"a.zummo@...ertech.it" <a.zummo@...ertech.it>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"rtc-linux@...glegroups.com" <rtc-linux@...glegroups.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Samuel Ortiz <sameo@...ux.intel.com>
Subject: RE: [PATCH v3 1/2] rtc: add new lp8788 rtc driver

Hi Devendra,

> > +static int lp8788_alarm_irq_enable(struct device *dev, unsigned int
> enable)
> > +{
> > +       struct lp8788_rtc *rtc = dev_get_drvdata(dev);
> > +       struct lp8788 *lp = rtc->lp;
> > +       u8 mask, shift;
> > +
> > +       mask = mask_alarm_en[rtc->alarm];
> > +       shift = shift_alarm_en[rtc->alarm];
> > +
> > +       return lp8788_update_bits(lp, LP8788_INTEN_3, mask, enable <<
> shift);
> 
> i have no idea about this function but just asking,
> 
> this will be called from ioctl right? so what if user calls RTC_AIE_ON
> and you return the IRQ status ? enable or disable like that?
> 
> i guess a failure in setting for no irq and a success otherwise.

Please go to my answer below - at the end of mail.

> 
> > +}
> > +
> > +static const struct rtc_class_ops lp8788_rtc_ops = {
> > +       .read_time = lp8788_rtc_read_time,
> > +       .set_time = lp8788_rtc_set_time,
> > +       .read_alarm = lp8788_read_alarm,
> > +       .set_alarm = lp8788_set_alarm,
> > +       .alarm_irq_enable = lp8788_alarm_irq_enable,
> > +};
> > +
> > +static irqreturn_t lp8788_alarm_irq_handler(int irq, void *ptr)
> > +{
> > +       struct lp8788_rtc *rtc = ptr;
> > +
> > +       rtc_update_irq(rtc->rdev, 1, ALARM_IRQ_FLAG);
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static int lp8788_alarm_irq_register(struct platform_device *pdev,
> > +                               struct lp8788_rtc *rtc)
> > +{
> > +       struct resource *r;
> > +       struct lp8788 *lp = rtc->lp;
> > +       struct irq_domain *irqdm = lp->irqdm;
> > +       int irq;
> > +
> > +       rtc->irq = 0;
> > +
> > +       if (!lp->pdata) {
> > +               dev_warn(lp->dev, "no platform data for rtc irq\n");
> 
> but we didn't registered irqs right? if we return 0 here means that we
> have a vaild irq no?

Oh, it's my mistake.
Alarm IRQ number is configured in the mfd-lp8788 driver and map it by irq-domain.
Therefore no platform data is required here.

> > +static int lp8788_rtc_probe(struct platform_device *pdev)
> > +{
> > +       struct lp8788 *lp = dev_get_drvdata(pdev->dev.parent);
> > +       struct lp8788_rtc *rtc;
> > +       struct device *dev = &pdev->dev;
> > +
> > +       rtc = devm_kzalloc(lp->dev, sizeof(struct lp8788_rtc),
> GFP_KERNEL);
> 
> lp->dev may be parent device, is &pdev->dev, or dev may be correct?

Great catch! You're correct. Thanks!

> 
> > +       if (!rtc)
> > +               return -ENOMEM;
> > +
> > +       rtc->lp = lp;
> > +       rtc->alarm = lp->pdata ? lp->pdata->alarm_sel :
> DEFAULT_ALARM_SEL;
> > +       platform_set_drvdata(pdev, rtc);
> > +
> > +       device_init_wakeup(dev, 1);
> > +
> > +       rtc->rdev = rtc_device_register("lp8788_rtc", dev,
> > +                                       &lp8788_rtc_ops, THIS_MODULE);
> > +       if (IS_ERR(rtc->rdev)) {
> > +               dev_err(dev, "can not register rtc device\n");
> 
>                     return PTR_ERR(rtc->rdev);  may be better?

Agree, thanks!

> 
> > +               return -ENODEV;
> > +       }
> > +
> > +       if (lp8788_alarm_irq_register(pdev, rtc))
> > +               dev_warn(lp->dev, "no rtc irq handler\n");
> > +
> 
> even if there is no alarm irq, we can still use the rtc to get the
> time, but there wont be RTC_AIE_ON ioctl enabled ?

If ALARM IRQ can't be registered, there are two handling conditions.

(a) Should it return as error in _probe() and no RTC is used anymore?
: Very simple handling but we can't set/get the RTC time even no problem.

(b) Just returns OK in _probe() and error handling when RTC_AIE_ON/OFF IOCTL is 
called?
: It guarantees RTC time functions but alarm IRQ doesn't work.
Please see the code below.

static int lp8788_alarm_irq_enable(struct device *dev, unsigned int enable)
{
	struct lp8788_rtc *rtc = dev_get_drvdata(dev);
	struct lp8788 *lp = rtc->lp;
	u8 mask, shift;

+	if (!rtc->irq)
+		return -EIO;

	mask = mask_alarm_en[rtc->alarm];
	shift = shift_alarm_en[rtc->alarm];

	return lp8788_update_bits(lp, LP8788_INTEN_3, mask, enable << shift);
}

'rtc->irq' is updated only when lp8788_alarm_irq_register() returns true.
If no alarm IRQ is specified, RTC_AIE_ON/OFF returns as EIO.
Then, RTC set/get time works well and alarm IRQ doesn't work.

I believe case (b) is better solution, but I'd like to have your opinion.
Thanks!

Best Regards,
Milo


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