On 03/19/2013 01:25:42 AM, Wang Dongsheng-B40534 wrote:
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, March 19, 2013 8:31 AM
> To: Wang Dongsheng-B40534
> Cc: Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org; Wang
Dongsheng-
> B40534; Zhao Chenhui-B35336; Li Yang-R58472
> Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
>
> On 03/08/2013 01:38:47 AM, Wang Dongsheng wrote:
> > +static ssize_t fsl_timer_wakeup_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf,
> > + size_t count)
> > +{
> > + struct timeval interval;
> > + int ret;
> > +
> > + interval.tv_usec = 0;
> > + if (kstrtol(buf, 0, &interval.tv_sec))
> > + return -EINVAL;
>
> I don't think the buffer will NUL-terminated... Ordinarily
there'll be
> an LF terminator, but you can't rely on that (many other sysfs
attributes
> seem to, though...).
>
I think we don't need to care about LF terminator.
The kstrtol--> _kstrtoull has been done.
My point is, what happens if userspace passes in a buffer that has no
terminator of any sort? kstrtol will continue reading beyond the end
of the buffer.
> > + mutex_lock(&sysfs_lock);
> > +
> > + if (fsl_wakeup->timer && !interval.tv_sec) {
> > + disable_irq_wake(fsl_wakeup->timer->irq);
> > + mpic_free_timer(fsl_wakeup->timer);
> > + fsl_wakeup->timer = NULL;
> > + mutex_unlock(&sysfs_lock);
> > +
> > + return count;
> > + }
> > +
> > + if (fsl_wakeup->timer) {
> > + mutex_unlock(&sysfs_lock);
> > + return -EBUSY;
> > + }
>
> So to change an already-set timer you have to set it to zero and
then to
> what you want? Why not just do:
>
> if (fsl_wakeup->timer) {
> disable_irq_wake(...);
> mpic_free_timer(...);
> fsl_wakeup_timer = NULL;
> }
>
> if (!interval.tv_sec) {
> mutex_unlock(&sysfs_lock);
> return count;
> }
>
You can't break up the it.
if echo zero the code will cancel the timer that is currently running.
Not echo non-zero value just zero to cancel.
Echoing a nonzero value wouldn't just be to cancel, it would be to set
a new timer after cancelling the old.
> > + for (i = 0; mpic_attributes[i]; i++) {
> > + ret = device_create_file(mpic_subsys.dev_root,
> > + mpic_attributes[i]);
> > + if (ret)
> > + goto err2;
> > + }
>
> Is this code ever going to register more than one?
>
No, just one. I only keep the style here.
If you don't think it's necessary I can remove this loop.
I don't think it's necessary.
-Scott
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
