Re: [RFC PATCH] PM / Runtime: runtime: Add sysfs option for forcing runtime suspend

[Oliver Neukum]

On Tue, 2015-09-08 at 01:10 +, Tirdea, Irina wrote:

> However, in the scenario I mentioned this is exactly what is happening.
> When turning off the screen of a mobile device, the user space

Would you explain why user space doesn't simply stop using those
devices, which in turn will make them idle? There are obvious cases
like keyboards or SCSI hosts where the kernel controls stuff, but could
you state where you expect this to be most useful?
Or why devices cannot be closed, e.g. you need to be sure settings
be not lost or is it something else?

> is the one that suspends the devices that are not needed in order to save 
> power
> (like touchscreens). Each such driver export an enable/disable attribute
> that calls the same code as resume/suspend (e.g. touchscreen drivers ads7846,
> ad7877 and most Android drivers not merged upstream). This adds more
> complexity to every driver by adding one more logical power state.
> It would be good to have a common interface instead of doing this in
> every 

Now these are two distinct questions.

1. a common interface
2. a capability implemented in common code

It is important to keep that apart. I suppose if we want this at all
#1 is a given. #2 however may be impossible in a generic manner

> I might have not used "forced" in the proper way here. What I mean by it is 
> that
> the device can be runtime suspended while ignoring the runtime usage count.

That is highly problematic. You'd need to audit the locking in every
driver. Right now elevating the count means that suspend()/resume()
cannot race with user space, as in the case of the system suspending
user space is frozen.

> In this implementation, user space is only allowed to change the states
> bottom-up in the sysfs hierarchy (it cannot force suspend a device if it
> has children that have not been suspended by user space).

That is obviously not enough. Take the worst case: we are flashing some
firmware. Or far more harmless: a key is has been pressed on a keyboard

> Would it work if this would be a capability that individual drivers need
> to declare?

For some drivers. But it needs support in the driver. Right now we can
make a device idle by calling close(). In fact we can benefit for
example in mice from this. But it needs support in the drivers.

> In the previous discussion thread , there were a couple of options
> mentioned, but none seemed to reach a consensus. You mentioned
> adding a "more aggressive runtime PM mode" [1]. I'm not sure how

That would have to be done on a per driver base.

> this would work except for adding a sysfs attribute that would trigger
> a runtime suspend while ignoring usage count. Would that be a
> better direction?

No. If we want this at all, we need a new callback to notify drivers
that user space is temporarily uninterested in a device. And the reverse
of course.
The power model is good. We must not assume that devices can be
suspended at will. If we do this at all, we ought to see it as giving
strong hints to drivers when a device can be considered idle.

Regards
Oliver


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

Re: [PATCH 2/2] usbnet: Fix a race between usbnet_stop() and the BH

[Oliver Neukum]

On Mon, 2015-08-31 at 11:50 +0300, Eugene Shatokhin wrote:
> > But I would have liked it much better if the code became simpler
> instead
> > of more complex.
> 
> Me too, but I can see no other way here. The code is simpler without 
> locking, indeed, but locking is needed to prevent the problems
> described 
> earlier.

On a practical note, will you resend the patch?

Regards
Oliver



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

Re: [PATCH 2/2] usbnet: Fix a race between usbnet_stop() and the BH

[Oliver Neukum]

On Fri, 2015-08-28 at 11:09 +0300, Eugene Shatokhin wrote:
> > Exactly what problem will that result in?  The tasklet_kill() will
> wait
> > for the processing of the single element done queue, and everything
> will
> > be fine.  Or?
> 
> Given enough time, what prevents defer_bh() from calling 
> tasklet_schedule(&dev->bh) *after* usbnet_stop() calls tasklet_kill()?
> 
> Consider the following situation (assuming '&&' are changed to '||'
> in 
> that while loop in usbnet_terminate_urbs() as they should be):
> 
> CPU0CPU1
> usbnet_stop()   defer_bh() with list == dev->rxq
>usbnet_terminate_urbs()
>  __skb_unlink() removes the last
>  skb from dev->rxq.
>  dev->rxq, dev->txq and dev->done
>  are now empty.
>while (!skb_queue_empty()...)
>  The loop ends because all 3
>  queues are now empty.
> 
>usbnet_terminate_urbs() ends.
> 
> usbnet_stop() continues:
>usbnet_status_stop(dev);
>...
>del_timer_sync (&dev->delay);
>tasklet_kill (&dev->bh);
>  __skb_queue_tail(&dev->done, skb);
>  if (dev->done.qlen == 1)
>tasklet_schedule(&dev->bh);
> 
> The BH is scheduled at this point, which is not what was intended.
> The 
> race window is small, but still.

This looks possible. I cannot come up with a better fix, although
it isn't nice. Thanks for finding this.

Regards
Oliver


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

Re: [PATCH 1/2] usbnet: Get EVENT_NO_RUNTIME_PM bit before it is cleared

[Oliver Neukum]

On Mon, 2015-08-24 at 23:13 +0300, Eugene Shatokhin wrote:
> It is needed to check EVENT_NO_RUNTIME_PM bit of dev->flags in
> usbnet_stop(), but its value should be read before it is cleared
> when dev->flags is set to 0.
> 
> The problem was spotted and the fix was provided by
> Oliver Neukum .
> 
> Signed-off-by: Eugene Shatokhin 
Acked-by: Oliver Neukum 

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

Re: [PATCH 1/2] usbnet: Get EVENT_NO_RUNTIME_PM bit before it is cleared

[Oliver Neukum]

On Mon, 2015-08-24 at 23:13 +0300, Eugene Shatokhin wrote:
> It is needed to check EVENT_NO_RUNTIME_PM bit of dev->flags in
> usbnet_stop(), but its value should be read before it is cleared
> when dev->flags is set to 0.

Can we agree that this at least is good and should go upstream
and into stable?

Regards
Oliver


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

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[Oliver Neukum]

On Mon, 2015-08-24 at 14:21 -0400, Alan Stern wrote:
> > In theory, an architecture could implement atomic bit operations
> using 
> > a spinlock to insure atomicity.  I don't know if any architectures
> do 
> > this, but if they do then the scenario above could arise.
> 
> Now that I see this in writing, I realize it's not possible after
> all.  
> clear_bit() et al. will work with a single unsigned long, which
> doesn't
> leave any place for spinlocks or other mechanisms.  I was thinking of 
> atomic_t.

Refuting yourself you are making the assumption that the lock has
to be inside the data structure. That is not true.

Regards
Oliver


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

Re: [PATCH] usbnet: Fix two races between usbnet_stop() and the BH

[Oliver Neukum]

On Mon, 2015-08-24 at 15:29 +0200, Bjørn Mork wrote:
> Eugene Shatokhin  writes:
> 
> > 19.08.2015 15:31, Bjørn Mork пишет:
> >> Eugene Shatokhin  writes:

> >> Stopping the tasklet rescheduling etc depends only on netif_running(),
> >> which will be false when usbnet_stop is called.  There is no need to
> >> touch dev->flags for this to happen.
> >
> > That was one of the first ideas we discussed here. Unfortunately, it
> > is probably not so simple.
> >
> > Setting dev->flags to 0 makes some delayed operations do nothing and,
> > among other things, not to reschedule usbnet_bh().
> 
> Yes, but I believe that is merely a side effect.  You should never need
> to clear multiple flags to get the desired behaviour.

Why? Is there any reason you cannot have a TX and an RX halt at the same
time?

> > As you can see in drivers/net/usb/usbnet.c, usbnet_bh() can be called
> > as a tasklet function and as a timer function in a number of
> > situations (look for the usage of dev->bh and dev->delay there).
> >
> > netif_running() is indeed false when usbnet_stop() runs, usbnet_stop()
> > also disables Tx. This seems to be enough for many cases where
> > usbnet_bh() is scheduled, but I am not so sure about the remaining
> > ones, namely:
> >
> > 1. A work function, usbnet_deferred_kevent(), may reschedule
> > usbnet_bh(). Looks like the workqueue is only stopped in
> > usbnet_disconnect(), so a work item might be processed while
> > usbnet_stop() works. Setting dev->flags to 0 makes the work function
> > do nothing, by the way. See also the comment in usbnet_stop() about
> > this.

Yes, this is the main reason the flags are collectively cleared.
We could do them all with clear_bit(). Ugly though.

> > A work item may be placed to this workqueue in a number of ways, by
> > both usbnet module and the mini-drivers. It is not too easy to track
> > all these situations.
> 
> That's an understatement :)

Yes.

> So FLAG_AVOID_UNLINK_URBS should probably be removed and replaced calls
> to usbnet_status_start() and usbnet_status_stop().  This will require
> testing on some of the devices with the original firmware problem
> however.

And there you point out the main problem.

> In any case: I do not think this flag should be considered when trying
> to make usbnet_stop behaviour saner.  It's only purpose is to
> deliberately break usbnet_stop by not actually stopping.

Yes.

Regards
Oliver


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

Re: [PATCH net v2 2/2] r8152: reset device when tx timeout

[Oliver Neukum]

On Wed, 2015-07-29 at 02:06 +, Hayes Wang wrote:
>  Oliver Neukum [mailto:oneu...@suse.com]
> > Sent: Tuesday, July 28, 2015 8:59 PM
> [...]
> > > > >  static void rtl8152_tx_timeout(struct net_device *netdev)  {
> > > > > struct r8152 *tp = netdev_priv(netdev);
> > > > > -   int i;
> > > > >
> > > > > netif_warn(tp, tx_err, netdev, "Tx timeout\n");
> > > > > -   for (i = 0; i < RTL8152_MAX_TX; i++)
> > > > > -   usb_unlink_urb(tp->tx_info[i].urb);
> > > > > +
> > > > > +   usb_queue_reset_device(tp->intf);
> > > > > +   cancel_delayed_work(&tp->schedule);
> > > >
> > > > Sorry to bother you again, but this looks wrong.
> > > > You want to cancel first. There is no point in running any work
> > > > before the reset is done. It will undo any progress anyway.
> > >
> > > Excuse me. Do you mean I don't need cancel the other work because it
> > > wouldn't be run before the reset is finished?
> > 
> > No, whatever the other work will do, the reset will undo.
> 
> Excuse me. I don't understand why I couldn't use usb_queue_reset_device() 
> directly.
> Why the reset will undo? 

Now, I think I got the reason for the confusion.

You are using cancel_delayed_work(&tp->schedule); after you queue
a reset. Therefore the order in which the work and the reset will
be executed is undefined. Usually the scheduled work will be canceled,
but not always.

That is not good.

Regards
Oliver


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

Re: [PATCH net v2 2/2] r8152: reset device when tx timeout

[Oliver Neukum]

On Tue, 2015-07-28 at 12:31 +, Hayes Wang wrote:
> Oliver Neukum [mailto:oneu...@suse.com]
> > Sent: Tuesday, July 28, 2015 8:14 PM
> [...]
> > >  static void rtl8152_tx_timeout(struct net_device *netdev)  {
> > > struct r8152 *tp = netdev_priv(netdev);
> > > -   int i;
> > >
> > > netif_warn(tp, tx_err, netdev, "Tx timeout\n");
> > > -   for (i = 0; i < RTL8152_MAX_TX; i++)
> > > -   usb_unlink_urb(tp->tx_info[i].urb);
> > > +
> > > +   usb_queue_reset_device(tp->intf);
> > > +   cancel_delayed_work(&tp->schedule);
> > 
> > Sorry to bother you again, but this looks wrong.
> > You want to cancel first. There is no point in running any work before the 
> > reset is
> > done. It will undo any progress anyway.
> 
> Excuse me. Do you mean I don't need cancel the other work because it wouldn't 
> be
> run before the reset is finished?

No, whatever the other work will do, the reset will undo.

Regards
Oliver


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

Re: [PATCH net v2 2/2] r8152: reset device when tx timeout

[Oliver Neukum]

On Tue, 2015-07-28 at 20:08 +0800, Hayes Wang wrote:
>  static void rtl8152_tx_timeout(struct net_device *netdev)
>  {
> struct r8152 *tp = netdev_priv(netdev);
> -   int i;
>  
> netif_warn(tp, tx_err, netdev, "Tx timeout\n");
> -   for (i = 0; i < RTL8152_MAX_TX; i++)
> -   usb_unlink_urb(tp->tx_info[i].urb);
> +
> +   usb_queue_reset_device(tp->intf);
> +   cancel_delayed_work(&tp->schedule);

Sorry to bother you again, but this looks wrong.
You want to cancel first. There is no point in
running any work before the reset is done. It will
undo any progress anyway.

Regards
Oliver


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

Re: [PATCH net 1/2] r8152: add pre_reset and post_reset

[Oliver Neukum]

On Tue, 2015-07-28 at 09:52 +, Hayes Wang wrote:
> Oliver Neukum [mailto:oneu...@suse.com]
> > Sent: Tuesday, July 28, 2015 4:53 PM
> [...]
> > > + return 0;
> > > +
> > > + netdev = tp->netdev;
> > > + if (!netif_running(netdev))
> > > + return 0;
> > > +
> > > + ret = usb_autopm_get_interface(intf);
> > > + if (ret < 0)
> > > + return ret;
> > 
> > What sense does this make?
> > 
> [...]
> > > + return 0;
> > > +
> > > + netdev = tp->netdev;
> > > + if (!netif_running(netdev))
> > > + return 0;
> > > +
> > > + ret = usb_autopm_get_interface(intf);
> > 
> > The device will be awake.
> 
> I don't sure if the device would be in runtimesuspend, so I wake it up by 
> myself.
> I think you mean I don't have to do this. I would remove them and resend the
> patch. Thanks.

Usbcore will resume the device.

HTH
Oliver

 A. 
/* Prevent autosuspend during the reset */
usb_autoresume_device(udev);

if (config) {
for (i = 0; i < config->desc.bNumInterfaces; ++i) {
struct usb_interface *cintf = config->interface[i];
struct usb_driver *drv;
int unbind = 0;

if (cintf->dev.driver) {
drv = to_usb_driver(cintf->dev.driver);
if (drv->pre_reset && drv->post_reset)
unbind = (drv->pre_reset)(cintf);




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

Re: [PATCH net 2/2] r8152: reset device when tx timeout

[Oliver Neukum]

On Tue, 2015-07-28 at 15:36 +0800, Hayes Wang wrote:
> The device reset is necessary if the hw becomes abnormal and stops
> transmitting packets.

You are not the first one to face this problem. Hence there
is a helper:

 * usb_queue_reset_device - Reset a USB device from an atomic context
 * @iface: USB interface belonging to the device to reset
 *
 * This function can be used to reset a USB device from an atomic
 * context, where usb_reset_device() won't work (as it blocks).

Please use it if you can. Your version for example is buggy.
It will oops if you unplug the device while a reset is scheduled.

Regards
Oliver


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

Re: [PATCH net 1/2] r8152: add pre_reset and post_reset

[Oliver Neukum]

On Tue, 2015-07-28 at 15:36 +0800, Hayes Wang wrote:
> Add rtl8152_pre_reset() and rtl8152_post_reset() which are used when
> calling usb_reset_device(). The two functions could reduce the time
> of reset when calling usb_reset_device() after probe().
> 
> Signed-off-by: Hayes Wang 
> ---
>  drivers/net/usb/r8152.c | 68 
> +
>  1 file changed, 68 insertions(+)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 144dc64..a6caa60 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -3342,6 +3342,72 @@ static void r8153_init(struct r8152 *tp)
>   r8153_u2p3en(tp, true);
>  }
>  
> +static int rtl8152_pre_reset(struct usb_interface *intf)
> +{
> + struct r8152 *tp = usb_get_intfdata(intf);
> + struct net_device *netdev;
> + int ret;
> +
> + if (intf->condition != USB_INTERFACE_BOUND || !tp)

If the interface weren't bound, you wouldn't be called.

> + return 0;
> +
> + netdev = tp->netdev;
> + if (!netif_running(netdev))
> + return 0;
> +
> + ret = usb_autopm_get_interface(intf);
> + if (ret < 0)
> + return ret;

What sense does this make?

> +
> + napi_disable(&tp->napi);
> + clear_bit(WORK_ENABLE, &tp->flags);
> + usb_kill_urb(tp->intr_urb);
> + cancel_delayed_work_sync(&tp->schedule);
> + if (netif_carrier_ok(netdev)) {
> + netif_stop_queue(netdev);
> + mutex_lock(&tp->control);
> + tp->rtl_ops.disable(tp);
> + mutex_unlock(&tp->control);
> + }
> +
> + usb_autopm_put_interface(intf);
> +
> + return 0;
> +}
> +
> +static int rtl8152_post_reset(struct usb_interface *intf)
> +{
> + struct r8152 *tp = usb_get_intfdata(intf);
> + struct net_device *netdev;
> + int ret;
> +
> + if (intf->condition != USB_INTERFACE_BOUND || !tp)

Again unnecessary

> + return 0;
> +
> + netdev = tp->netdev;
> + if (!netif_running(netdev))
> + return 0;
> +
> + ret = usb_autopm_get_interface(intf);

The device will be awake.

> + if (ret < 0)
> + return ret;
> +
> + set_bit(WORK_ENABLE, &tp->flags);
> + if (netif_carrier_ok(netdev)) {
> + mutex_lock(&tp->control);
> + tp->rtl_ops.enable(tp);
> + rtl8152_set_rx_mode(netdev);
> + mutex_unlock(&tp->control);
> + netif_wake_queue(netdev);
> + }
> +
> + napi_enable(&tp->napi);
> +
> + usb_autopm_put_interface(intf);
> +
> + return ret;
> +}
> +

HTH
Oliver


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

Re: Several races in "usbnet" module (kernel 4.1.x)

[Oliver Neukum]

On Fri, 2015-07-24 at 20:38 +0300, Eugene Shatokhin wrote:
> 21.07.2015 15:04, Oliver Neukum пишет:

> > your analysis is correct and it looks like in addition to your proposed
> > fix locking needs to be simplified and a common lock to be taken.
> > Suggestions?
> 
> Just an idea, I haven't tested it.
> 
> How about moving the operations with dev->done under &list->lock in 
> defer_bh, while keeping dev->done.lock too and changing 

Why keep dev->done.lock?
Does it make sense at all?

> usbnet_terminate_urbs() as described below?
> 
> Like this:
> @@ -428,12 +428,12 @@ static enum skb_state defer_bh(struct usbnet *dev, 
> struct sk_buff *skb,
>   old_state = entry->state;
>   entry->state = state;
>   __skb_unlink(skb, list);
> - spin_unlock(&list->lock);
>   spin_lock(&dev->done.lock);
>   __skb_queue_tail(&dev->done, skb);
>   if (dev->done.qlen == 1)
>   tasklet_schedule(&dev->bh);
> - spin_unlock_irqrestore(&dev->done.lock, flags);
> + spin_unlock(&dev->done.lock);
> + spin_unlock_irqrestore(&list->lock, flags);
>   return old_state;
>   }
> ---
> 
> usbnet_terminate_urbs() can then be changed as follows:
> 
> @@ -749,6 +749,20 @@ EXPORT_SYMBOL_GPL(usbnet_unlink_rx_urbs);
> 
>  
> /*-*/
> 
> +static void wait_skb_queue_empty(struct sk_buff_head *q)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&q->lock, flags);
> + while (!skb_queue_empty(q)) {
> + spin_unlock_irqrestore(&q->lock, flags);
> + schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
> + set_current_state(TASK_UNINTERRUPTIBLE);

I suppose you want to invert those lines

> + spin_lock_irqsave(&q->lock, flags);
> + }
> + spin_unlock_irqrestore(&q->lock, flags);
> +}
> +

Your changes make sense, but it locks to me as if a lock would
become totally redundant.

Regards
Oliver


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

Re: Several races in "usbnet" module (kernel 4.1.x)

[Oliver Neukum]

On Fri, 2015-07-24 at 17:41 +0300, Eugene Shatokhin wrote:
> 23.07.2015 12:15, Oliver Neukum пишет:

>  From what I see now in Documentation/atomic_ops.txt, stores to the 
> properly aligned memory locations are in fact atomic.

They are, but again only with respect to each other.

> So, I think, the situation you described above cannot happen for 
> dev->flags, which is good. No need to address that in the patch. The 
> race might be harmless after all.
> 
> If I understand the code correctly now, dev->flags is set to 0 in 
> usbnet_stop() so that the worker function (usbnet_deferred_kevent) would

Yes, particularly not reschedule itself.

> do nothing, should it start later. If so, how about adding memory 
> barriers for all CPUs to see dev->flags is 0 before other things?

Taking a lock, as del_timer_sync() does, implies a memory barrier,
as does a work.

Regards
Oliver


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

Re: [PATCH 3/4] cp210x: Store part number

[Oliver Neukum]

On Mon, 2015-07-27 at 08:50 +0200, Petr Tesarik wrote:
> I don't understand. While you're right that I copied this part from
> Sillicon Labs' driver without much thinking, and &spriv->bPartNumber
> can be used directly, I can't see any DMA on stack. FWIW
> cp210x_control_msg always allocates a buffer using kcalloc:
> 
> buf = kcalloc(length, sizeof(__le32), GFP_KERNEL);
> /* ... */
> result = usb_control_msg(serial->dev,
> usb_rcvctrlpipe(serial->dev, 0),
>  request, requesttype, value,
>  spriv->bInterfaceNumber, buf, size,
> timeout);
> 
> Is that what you mean?

Yes, sorry, that part wasn't so clear from the previous patch.

Sorry
Oliver


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

Re: [PATCH 3/4] cp210x: Store part number

[Oliver Neukum]

On Fri, 2015-07-24 at 08:48 +0200, Petr Tesarik wrote:
> @@ -872,6 +886,12 @@ static int cp210x_startup(struct usb_serial
> *serial)
>  
> usb_set_serial_data(serial, spriv);
>  
> +   /* Get the 1-byte part number of the cp210x device */
> +   cp210x_control_msg(serial->port[0], CP210X_VENDOR_SPECIFIC,
> +  REQTYPE_DEVICE_TO_HOST, CP210X_GET_PARTNUM,
> +  &partnum, 1, USB_CTRL_GET_TIMEOUT);
> +   spriv->bPartNumber = partnum & 0xFF;

DMA on the stack. That breaks the cache coherency rules.
You must kmalloc the buffer.

Regards
Oliver


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

Re: [PATCH net 2/3] r8152: fix remote wakeup

[Oliver Neukum]

On Thu, 2015-07-23 at 09:55 +, Hayes Wang wrote:
> > Hi,
> > 
> > this is most likely wrong. Usbcore does check for a device's ability to
> > do remote wakeup and will block a runtime suspend if it detects that
> > a remote wakeup would be required but the device cannot deliver.
> > (static int autosuspend_check())
> > 
> > So by removing the flag in the probe() method means that devices will
> > suspend during operations without remote wakeup requested. Thus an
> > incoming packet cannot wake them up.
> > 
> > If you remove setting the flag on probe() you need to set it at open()
> > [and reset on close()], as devices which cannot do remote wakeup must
> > only be suspended when they are down.
> 
> Hi,
> 
> I don't think I understand your description clearly. My idea is that if the

Sorry, I will try to be clearer.

> device doesn't support wakeup, we don't need set " needs_remote_wakeup".

The algorithm for power saving needs remote wakeup.

> We allow the device could be suspended and couldn't be waked up by
> incoming packet. The system could be waked up by other methods except
> by the device.

The system is not the problem. I was talking about device level power
management at runtime.

> I don't understand why I have to set it at open() and reset it at close(). And
> why must the device only be suspended when it is down?

OK,

the driver right now requests that runtime power management be done:

static struct usb_driver rtl8152_driver = {
.name = MODULENAME,
.id_table = rtl8152_table,
.probe =rtl8152_probe,
.disconnect =   rtl8152_disconnect,
.suspend =  rtl8152_suspend,
.resume =   rtl8152_resume,
.reset_resume = rtl8152_resume,
.supports_autosuspend = 1,

^ the crucial flag

.disable_hub_initiated_lpm = 1,
};

It implements an advanced form of runtime power management which
requires remote wakeup.
For the device to work correctly it must not be suspended while

a - a setting is changed
b - a packet is transmitted
c - a packet is received

Cases a and b are covered by the driver on its own.
Case a see for example rtl8152_set_mac_address()
Case b is handled in rtl8152_start_xmit()

if (test_bit(SELECTIVE_SUSPEND, &tp->flags)) {
set_bit(SCHEDULE_NAPI, &tp->flags);
schedule_delayed_work(&tp->schedule, 0);
} else {
usb_mark_last_busy(tp->udev);

The scheduled work will resume the device if necessary.

Case c is handled in rtl8152_resume()

if (netif_running(tp->netdev)) {
if (test_bit(SELECTIVE_SUSPEND, &tp->flags)) {
rtl_runtime_suspend_enable(tp, false);
clear_bit(SELECTIVE_SUSPEND, &tp->flags);
set_bit(WORK_ENABLE, &tp->flags);
if (netif_carrier_ok(tp->netdev))
rtl_start_rx(tp);

But that code path only runs if remote wakeup is enabled.
So to operate with runtime power management is is necessary
that the device support remote wakeup and the driver enable it.

If the device does not support remote wakeup and the driver
enables it, runtime power management will be switched off.
That is the current state and it means that devices which
don't support remote wakeup cannot do runtime power management
at all. But the driver is correct.

The only time a device that doesn't support remote wakeup can
do runtime power managent is when no packets can be received
that is while the interface is down. If you want to allow that
you must not set needs_remote_wakeup in probe(), but you must
set it in open() because it is necessary for runtime power
management as I explained above.

Sorry for the length of this mail, but I wanted to make sure
I am absolutely clear this time.

HTH
Oliver


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

Re: Several races in "usbnet" module (kernel 4.1.x)

[Oliver Neukum]

On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:
> [Race #5]
> 
> Race on dev->rx_urb_size. I reproduced it a similar way as the races
> #2 
> and #3 (changing MTU while downloading files).
> 
> dev->rx_urb_size is written to here:
> #0  usbnet_change_mtu (usbnet.c:392)
> dev->rx_urb_size = dev->hard_mtu;
> 
> Here is the conflicting read from dev->rx_urb_size:
> * rx_submit (usbnet.c:467)
> size_t  size = dev->rx_urb_size;

Yes, but what is the actual bad race? I mean, if you change
the MTU in operation, there will be a race. That is just
unavoidable.
Do we generate errors?

Regards
Oliver


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

Re: Several races in "usbnet" module (kernel 4.1.x)

[Oliver Neukum]

On Wed, 2015-07-22 at 21:33 +0300, Eugene Shatokhin wrote:
> The following part is not necessary, I think. usbnet_bh() does not
> touch 
> EVENT_NO_RUNTIME_PM bit explicitly and these bit operations are
> atomic 
> w.r.t. each other.
> 
> > + mpn |= !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
> > + /* in case the bh reset a flag */

Yes, they are atomic w.r.t. each other. And that limitation worries me.

I am considering architectures which do atomic operations with
spinlocks. And this code mixes another operation into it. Can
this happen?

CPU A   CPU B

take lock
read old value
set value to 0
clear bit
write back changed value
release lock

Regards
Oliver


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

Re: [PATCH net 2/3] r8152: fix remote wakeup

[Oliver Neukum]

On Thu, 2015-07-23 at 15:09 +0800, Hayes Wang wrote:
> Set needs_remote_wakeup only when the device supports it.
> 
> Signed-off-by: Hayes Wang 
> ---
>  drivers/net/usb/r8152.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index e3a0110..eff1f25 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -4059,7 +4059,8 @@ static int rtl8152_probe(struct usb_interface *intf,
>   break;
>   }
>  
> - intf->needs_remote_wakeup = 1;
> + if (udev->actconfig->desc.bmAttributes & USB_CONFIG_ATT_WAKEUP)
> + intf->needs_remote_wakeup = 1;

Hi,

this is most likely wrong. Usbcore does check for a device's ability to
do remote wakeup and will block a runtime suspend if it detects that
a remote wakeup would be required but the device cannot deliver.
(static int autosuspend_check())

So by removing the flag in the probe() method means that devices will
suspend during operations without remote wakeup requested. Thus an
incoming packet cannot wake them up.

If you remove setting the flag on probe() you need to set it at open()
[and reset on close()], as devices which cannot do remote wakeup must
only be suspended when they are down.

Regards
Oliver



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

Re: [PATCH] Fix data loss in cdc-acm

[Oliver Neukum]

On Wed, 2015-07-22 at 10:30 -0400, Peter Hurley wrote:
> 3. Pre-allocate space _before_ the data arrives (with
> tty_buffer_request_room());
>this is applicable to subsystems which know how much data can be
> in-flight
>at any one time. This guarantees that when rx data arrives buffer
> space is
>available (since it has already been allocated).
> 
> Drivers that use method 2 typically attempt to recopy the buffered
> data
> when either new data arrives or @ unthrottle. I've seen others use
> deferred
> work as well.
> 
> AFAIK no driver/subsystem is using method 3 for guaranteed delivery
> of in-flight data, but it seems ideally suited to usb serial.

Indeed. But flow control is still done by throttle/unthrottle, isn't it?

Regards
Oliver


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

Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Oliver Neukum]

On Wed, 2015-07-22 at 03:25 +0200, Rafael J. Wysocki wrote:
> And it is more pain for me to change the user space on each of them to
> write to the new sysfs file on every boot than to set a kernel Kconfig
> option once.

So why at all? If you really need this in sysfs, why not write
something like "memfast" into /sys/power/state ?

Regards
Oliver


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

Re: [PATCH] Fix data loss in cdc-acm

[Oliver Neukum]

On Tue, 2015-07-21 at 12:45 -0400, Peter Hurley wrote:
> Let me know if you need help instrumenting the tty buffers/throttling
> to help figure out what the actual problem is.
> 
> Regarding the patch itself, I have no opinion on the suitability of
> simply not resubmitting urbs. However, that is exactly how the
> throttle
> mechanism works, and the tty buffer API is specifically designed to
> allow drivers to manage flow via that interface as well (especially
> for high-throughput drivers).

Could you please expand on how this is supposed to work?
For once how does one learn that room is available again?

Regards
Oliver


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

Re: Several races in "usbnet" module (kernel 4.1.x)

[Oliver Neukum]

On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:
> And here, the code clears EVENT_RX_KILL bit in dev->flags, which may 
> execute concurrently with the above operation:
> #0 clear_bit (bitops.h:113, inlined)
> #1 usbnet_bh (usbnet.c:1475)
> /* restart RX again after disabling due to high error rate */
> clear_bit(EVENT_RX_KILL, &dev->flags);
> 
> If clear_bit() is atomic w.r.t. setting dev->flags to 0, this race is 
> not a problem, I guess. Otherwise, it may be.

clear_bit is atomic with respect to other atomic operations.
So how about this:

Regards
Oliver

>From 1c4e685b3a9c183e04c46b661830e5c7ed35b513 Mon Sep 17 00:00:00 2001
From: Oliver Neukum 
Date: Tue, 21 Jul 2015 16:19:40 +0200
Subject: [PATCH] usbnet: fix race between usbnet_stop() and the BH

Does this do the job?

Signed-off-by: Oliver Neukum 
---
 drivers/net/usb/usbnet.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 3c86b10..77a9a86 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -778,7 +778,7 @@ int usbnet_stop (struct net_device *net)
 {
struct usbnet   *dev = netdev_priv(net);
struct driver_info  *info = dev->driver_info;
-   int retval, pm;
+   int retval, pm, mpn;
 
clear_bit(EVENT_DEV_OPEN, &dev->flags);
netif_stop_queue (net);
@@ -813,14 +813,17 @@ int usbnet_stop (struct net_device *net)
 * can't flush_scheduled_work() until we drop rtnl (later),
 * else workers could deadlock; so make workers a NOP.
 */
+   mpn = !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
dev->flags = 0;
del_timer_sync (&dev->delay);
tasklet_kill (&dev->bh);
+   mpn |= !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
+   /* in case the bh reset a flag */
+   dev->flags = 0;
if (!pm)
usb_autopm_put_interface(dev->intf);
 
-   if (info->manage_power &&
-   !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags))
+   if (info->manage_power && mpn)
info->manage_power(dev, 0);
else
usb_autopm_put_interface(dev->intf);
-- 
2.1.4



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

Re: [PATCH] Fix data loss in cdc-acm

[Oliver Neukum]

On Mon, 2015-07-20 at 20:07 +0200, Sven Brauch wrote:
> On 20/07/15 19:25, Johan Hovold wrote:
> > What kernel version are you using?
> I'm using linux 4.1.2.
> 
> > The idea of adding another layer of buffering in the cdc-acm driver has
> > been suggested in the past but was rejected (or at least questioned).
> > See for example this thread:
> > 
> > https://lkml.kernel.org/r/20110608164626.22bc8...@lxorguk.ukuu.org.uk
> Yes, that is indeed pretty much the same problem and the same solution.
> Answering to the questions brought up in that thread:

Yes, but that was not really a modem. The stuff on the other end
doesn't come over an external conection.

> > a) Why is your setup filling 64K in the time it takes the throttle
> > response to occur
> As far as I understand, the throttle happens only when there's less than
> 128 bytes of free space in the tty buffer. Data can already be lost
> before the tty even decides it should start throttling, simply because
> the throttle threshold is smaller than the amount of data potentially in
> each urb. Also (excuse my cluelessness) it seems that when exactly the

Then there is a problem in the tty code. Now it may not have enough
information, but there is no point of turning the buffers of cdc-acm
into an inofficial buffer.

> throttling happens depends on some scheduling "jitter" as well.
> Additionally, the response of the cdc_acm driver to a throttle request
> is not very prompt; it might have a queue of up to 16kB (16 urbs) pending.
> 
> > b) Do we care (is the right thing to do to lose bits anyway at
> > that point)
> This I cannot answer, I don't know enough about the architecture or
> standards. I can just say that for my case, there's a lot of losses;
> this it not an issue which happens after hours when the system is under
> heavy load, it happens after just a few seconds reproducably.

Then the tty layer needs to throttle earlier. In the general case we
must be ready to throw away data.

> > The tty buffers are quite large these days, but could possibly be bumped
> > further if needed to give the ldisc some more time to throttle the
> > device at very high speeds.
> I do not like this solution. It will again be based on luck, and you
> will still be unable to rely on the delivery guarantee made by the USB
> stack (at least when using bulk).
> My suggestion instead stops the host system from accepting any more data
> from the device when its buffers are full, forcing the device to wait
> before sending out more data (which many kinds of devices might very
> well be able to do).

But others won't and we'd preserve stale data in preference over fresh
data.

> Also note that this patch does not introduce an extra layer of
> buffering. The buffers are already there; this change just alters the
> process which decides when to submit the buffers to the tty, and when to
> free them for more input data from the device.

It does. It turns those buffers from temporary scratch buffers into
a queue.

Regards
Oliver


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

Re: Several races in "usbnet" module (kernel 4.1.x)

[Oliver Neukum]

On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:
> Races on dev->rx_qlen. Reproduced these by repeatedly changing MTU
> (1500 
> <-> 1400) while downloading large files.

Hi,

I don't see how it matters much. The number of buffers is just
an optimization. As long as it eventually is corrected I don't
see harm.

Regards
Oliver



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

Re: Several races in "usbnet" module (kernel 4.1.x)

[Oliver Neukum]

On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:
> Hi,
> 
> I have recently found several data races in "usbnet" module, checked on 
> vanilla kernel 4.1.0 on x86_64. The races do actually happen, I have 
> confirmed it by adding delays and using hardware breakpoints to detect 
> the conflicting memory accesses (with RaceHound tool, 
> https://github.com/winnukem/racehound).
> 
> I have not analyzed yet how harmful these races are (if they are), but 
> it is better to report them anyway, I think.
> 
> Everything was checked using YOTA 4G LTE Modem that works via "usbnet" 
> and "cdc_ether" kernel modules.
> --
> 
> [Race #1]
> 
> Race on skb_queue ('next' pointer) between usbnet_stop() and rx_complete().
> 
> Reproduced that by unplugging the device while the system was 
> downloading a large file from the Net.
> 
> Here is part of the call stack with the code where the changes to the 
> queue happen:
> 
> #0 __skb_unlink (skbuff.h:1517)   
>   prev->next = next;
> #1 defer_bh (usbnet.c:430)
>   spin_lock_irqsave(&list->lock, flags);
>   old_state = entry->state;
>   entry->state = state;
>   __skb_unlink(skb, list);
>   spin_unlock(&list->lock);
>   spin_lock(&dev->done.lock);
>   __skb_queue_tail(&dev->done, skb);
>   if (dev->done.qlen == 1)
>   tasklet_schedule(&dev->bh);
>   spin_unlock_irqrestore(&dev->done.lock, flags);
> #2 rx_complete (usbnet.c:640)
>   state = defer_bh(dev, skb, &dev->rxq, state);
> 
> At the same time, the following code repeatedly checks if the queue is 
> empty and reads the same values concurrently with the above changes:
> 
> #0  usbnet_terminate_urbs (usbnet.c:765)
>   /* maybe wait for deletions to finish. */
>   while (!skb_queue_empty(&dev->rxq)
>   && !skb_queue_empty(&dev->txq)
>   && !skb_queue_empty(&dev->done)) {
>   schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
>   set_current_state(TASK_UNINTERRUPTIBLE);
>   netif_dbg(dev, ifdown, dev->net,
> "waited for %d urb completions\n", temp);
>   }
> #1  usbnet_stop (usbnet.c:806)
>   if (!(info->flags & FLAG_AVOID_UNLINK_URBS))
>   usbnet_terminate_urbs(dev);
> 
> For example, it is possible that the skb is removed from dev->rxq by 
> __skb_unlink() before the check "!skb_queue_empty(&dev->rxq)" in 
> usbnet_terminate_urbs() is made. It is also possible in this case that 
> the skb is added to dev->done queue after "!skb_queue_empty(&dev->done)" 
> is checked. So usbnet_terminate_urbs() may stop waiting and return while 
> dev->done queue still has an item.

Hi,

your analysis is correct and it looks like in addition to your proposed
fix locking needs to be simplified and a common lock to be taken.
Suggestions?

Regards
Oliver


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

Re: [RFC ebeam PATCH 2/2] input: misc: New USB eBeam input driver

[Oliver Neukum]

On Mon, 2015-07-20 at 23:03 +0200, Yann Cantin wrote:
> Signed-off-by: Yann Cantin 
> ---
>  Documentation/ABI/testing/sysfs-driver-ebeam |  53 ++
>  drivers/input/misc/Kconfig   |  22 +
>  drivers/input/misc/Makefile  |   1 +
>  drivers/input/misc/ebeam.c   | 777 
> +++
>  4 files changed, 853 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-ebeam
>  create mode 100644 drivers/input/misc/ebeam.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-ebeam 
> b/Documentation/ABI/testing/sysfs-driver-ebeam
> new file mode 100644
> index 000..6873db5
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-ebeam
> @@ -0,0 +1,53 @@
> +What:/sys/class/input/inputXX/device/min_x
> + /sys/class/input/inputXX/device/min_y
> + /sys/class/input/inputXX/device/max_x
> + /sys/class/input/inputXX/device/max_y
> +Date:Jul 2015
> +Kernel Version:  4.1
> +Contact: yann.can...@laposte.net
> + linux-...@vger.kernel.org
> +Description:
> + Reading from these files return the actually used range values 
> of
> + the reported coordinates.
> + Writing to these files preset these range values.
> + See below for the calibration procedure.
> +
> +What:/sys/class/input/inputXX/device/h[1..9]
> +Date:Jul 2015
> +Kernel Version:  4.1
> +Contact: yann.can...@laposte.net
> + linux-...@vger.kernel.org
> +Description:
> + Reading from these files return the 3x3 transformation matrix 
> elements
> + actually used, in row-major.
> + Writing to these files preset these elements values.
> + See below for the calibration procedure.
> +
> +What:/sys/class/input/inputXX/device/calibrated
> +Date:Jul 2015
> +Kernel Version:  4.1
> +Contact: yann.can...@laposte.net
> + linux-...@vger.kernel.org
> +Description:
> + Reading from this file :
> + - Return 0 if the driver is in "un-calibrated" mode : it 
> actually send
> +   raw coordinates in the device's internal coordinates system.
> + - Return 1 if the driver is in "calibrated" mode : it send 
> computed coordinates
> +   that (hopefully) matches the screen's coordinates system.
> + Writing 1 to this file enable the "calibrated" mode, 0 reset 
> the driver in
> + "un-calibrated" mode.
> +
> +Calibration procedure :
> +
> +When loaded, the driver is in "un-calibrated" mode : it send device's raw 
> coordinates
> +in the [0..65535]x[0..65535] range, the transformation matrix is the 
> identity.
> +
> +A calibration program have to compute a homography transformation matrix 
> that convert
> +the device's raw coordinates to the matching screen's ones.
> +It then write to the appropriate sysfs files the computed values, 
> pre-setting the
> +driver's parameters : xy range, and the matrix's elements.
> +When all values are passed, it write 1 to the calibrated sysfs file to 
> enable the "calibrated" mode.
> +
> +Warning : The parameters aren't used until 1 is writen to the calibrated 
> sysfs file.
> +
> +Writing 0 to the calibrated sysfs file reset the driver in "un-calibrated" 
> mode.
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index d4f0a81..22c46a4 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -103,6 +103,28 @@ config INPUT_E3X0_BUTTON
> To compile this driver as a module, choose M here: the
> module will be called e3x0_button.
>  
> +config INPUT_EBEAM_USB
> + tristate "USB eBeam driver"
> + depends on USB_ARCH_HAS_HCD
> + select USB
> + help
> +   Say Y here if you have a USB eBeam pointing device and want to
> +   use it without any proprietary user space tools.
> +
> +   Have a look at  for
> +   a usage description and the required user-space tools.
> +
> +   Supported devices :
> + - Luidia eBeam Classic Projection and eBeam Edge Projection
> + - Nec NP01Wi1 & NP01Wi2 interactive solution
> +
> +   Supposed working devices, need test, may lack functionality :
> + - Luidia eBeam Edge Whiteboard and eBeam Engage
> + - Hitachi Starboard FX-63, FX-77, FX-82, FX-77GII
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called ebeam.
> +
>  config INPUT_PCSPKR
>   tristate "PC Speaker support"
>   depends on PCSPKR_PLATFORM
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 53df07d..125f8a9 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_INPUT_DA9055_ONKEY)+= da9055_onkey.o
>  obj-$(CONFIG_INPUT_DA9

Re: [PATCH] power: Destroy IDRs on module exit

[Oliver Neukum]

On Tue, 2015-07-14 at 11:16 +0200, Johannes Thumshirn wrote:
> Destroy IDRs on module exit, freeing the resources for
> * bq2415x_charger.c
> * ds2782_battery.c
> * ltc2941-battery-gauge.c
> 
> The drivers had to be converted to "ordinary"
> module_init()/module_exit()
> style drivers instead of using module_i2c_driver.

That is a drawback. How about adding a pointer to the idr
to "struct i2c_driver" and destroy it in generic code?

Regards
Oliver


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

Re: [PATCH] cdc-acm: Destroy acm_minors IDR on module exit

[Oliver Neukum]

On Wed, 2015-07-08 at 17:25 +0200, Johannes Thumshirn wrote:
> Destroy acm_minors IDR on module exit, reclaiming the allocated
> memory.
> 
> This was detected by the following semantic patch (written by Luis
> Rodriguez
> )
> 
> @ defines_module_init @
> declarer name module_init, module_exit;
> declarer name DEFINE_IDR;
> identifier init;
> @@
> 
> module_init(init);
> 
> @ defines_module_exit @
> identifier exit;
> @@
> 
> module_exit(exit);
> 
> @ declares_idr depends on defines_module_init && defines_module_exit @
> identifier idr;
> @@
> 
> DEFINE_IDR(idr);
> 
> @ on_exit_calls_destroy depends on declares_idr && defines_module_exit
> @
> identifier declares_idr.idr, defines_module_exit.exit;
> @@
> 
> exit(void)
> {
>  ...
>  idr_destroy(&idr);
>  ...
> }
> 
> @ missing_module_idr_destroy depends on declares_idr &&
> defines_module_exit && !on_exit_calls_destroy @
> identifier declares_idr.idr, defines_module_exit.exit;
> @@
> 
> exit(void)
> {
>  ...
>  +idr_destroy(&idr);
>  }
> 
> 
> Signed-off-by: Johannes Thumshirn 
Acked-by: Oliver Neukum 


-- 
Oliver Neukum 

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

Re: [PATCH 1/1] suspend: delete sys_sync()

[Oliver Neukum]

On Thu, 2015-07-09 at 00:03 +0200, Rafael J. Wysocki wrote:

> Nothing and I'm not discussing that (I've said that already at least once in
> this thread).
> 
> What I'm questioning is the "why" of calling sys_sync() from the kernel.

That's strictly speaking two questions

1. Why do it in the kernel

That is easy. It closes the window of a race condition.

2. Why do it at all

In essence because the system becomes inactive. For example we say that
data hits the disk after 30s maximum. We cannot meet such a limit unless
we sync.
There are additional issues, such as a system appearing inactive during
suspension and frankly the far greater likelihood of a crash.

Regards
Oliver


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

Re: [PATCH 1/1] suspend: delete sys_sync()

[Oliver Neukum]

On Wed, 2015-07-08 at 00:11 +0200, Rafael J. Wysocki wrote:
> On Tuesday, July 07, 2015 04:38:26 PM Oliver Neukum wrote:
>  
> > That is a tough nut. But that's not a reason to make it worse.
> > I'd say there's no reason not to use a secondary interface to
> > suspend without syncing or to extend or introduce such an interface
> > if the API is deficient.
> 
> Well, the point here is that the sync we have doesn't prevent all potentially
> possible bad things from happening.  It's a partial measure at best in that
> respect.

Well, removed hardware doesn't work. That is a very basic limitation.
But can we guarantee that any returned syscall actually wrote to disk?
Yes, but it must be done in kernel space. So doing a sync has a true
benefit.
I don't see why you would want to generally remove it. What is wrong
with an interface allowing a selection there to those who don't care
about additional guarantees?

Regards
Oliver


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

Re: [PATCH 1/1] suspend: delete sys_sync()

[Oliver Neukum]

On Tue, 2015-07-07 at 16:32 +0200, Rafael J. Wysocki wrote:
> On Tuesday, July 07, 2015 03:16:48 PM Oliver Neukum wrote:
> > On Tue, 2015-07-07 at 14:14 +0200, Rafael J. Wysocki wrote:
> > > For example, on desktop systems I use user space syncs filesystems
> > > before
> > > writing to /sys/power/state, so the additional sys_sync() in the
> > > kernel doesn't
> > > seem to serve any purpose.
> > 
> > There is a race you cannot close in user space.
> 
> Yes, there is, but I'm not sure how much of a help the sync in the kernel
> provides here anyway.
> 
> Say this happens.  There is a process writing to a file running in parallel
> with the suspend process.  Suspend starts and that process is frozen.  The
> sync is called and causes all of the outstanding data to be written back.
> The user doesn't realize that the write is technically still in progress, so

Well, in that case the user never got the feedback that the write is
finished. That is a race that always exists, like sending SIGKILL to a
running task.
What you describe is in principle unsolvable every time under
any circumstances.

> he (or she) pulls the storage device out of the system, moves it to another
> system, makes changes (say removes the file written to by the process above,
> so the blocks previously occupied by that file are now used for some metadata)
> and moves the storage back to the suspended system.  The system is resumed
> and the writing process continues writing possibly to the wrong blocks and
> corrupts the filesystem.

That is a tough nut. But that's not a reason to make it worse.
I'd say there's no reason not to use a secondary interface to
suspend without syncing or to extend or introduce such an interface
if the API is deficient.

Regards
Oliver


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

Re: [PATCH 1/1] suspend: delete sys_sync()

[Oliver Neukum]

On Tue, 2015-07-07 at 14:14 +0200, Rafael J. Wysocki wrote:
> For example, on desktop systems I use user space syncs filesystems
> before
> writing to /sys/power/state, so the additional sys_sync() in the
> kernel doesn't
> seem to serve any purpose.

There is a race you cannot close in user space.

Regards
Oliver


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

Re: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Oliver Neukum]

On Fri, 2015-07-03 at 10:51 +0200, Krzysztof Opasiak wrote:
> Doesn't we have the same problem with functionfs/gadgetfs and
> dummy_hcd? 
> Or with fuse?
> 
> It's a very generic problem for all "virtualized devices" and it is 
> known for quite a long time. This is why many tutorials about swap
> warns 
> that swap should be set up only on real block devices which are fully 
> served in kernel.

Indeed. But the point is that it isn't limited to swap. As you as
a page needs to be laundered the problem exists.
Mmapping a file for write (non private) is enough.

Regards
Oliver


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

Re: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Oliver Neukum]

On Thu, 2015-07-02 at 10:57 -0500, Jeremy White wrote:
> On 07/02/2015 07:10 AM, Oliver Neukum wrote:
> > On Thu, 2015-07-02 at 13:35 +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 02-07-15 10:45, Oliver Neukum wrote:
> >>> On Wed, 2015-07-01 at 10:06 +0100, Daniel P. Berrange wrote:
> >>>
> >>>> I don't really think it is sensible to be defining & implementing new
> >>>> network services which can't support strong encryption and 
> >>>> authentication.
> >>>> Rather than passing the file descriptor to the kernel and having it do
> >>>> the I/O directly, I think it would be better to dissassociate the kernel
> >>>> from the network transport, and thus leave all sockets layer data I/O
> >>>> to userspace daemons so they can layer in TLS or SASL or whatever else
> >>>> is appropriate for the security need.
> >>>
> >>> Hi,
> >>>
> >>> this hits a fundamental limit. Block IO must be done entirely in kernel
> >>> space or the system will deadlock. The USB stack is part of the block
> >>> layer and the SCSI error handling. Thus if you involve user space you
> >>> cannot honor memory allocation with GFP_NOFS and you break all APIs
> >>> where we pass GFP_NOIO in the USB stack.
> >>>
> >>> Supposed you need to reset a storage device for error handling.
> >>> Your user space programm does a syscall, which allocates memory
> >>> and needs to launder pages. It proceeds to write to the storage device
> >>> you wish to reset.
> >>>
> >>> It is the same problem FUSE has with writable mmap. You cannot do
> >>> block devices in user space sanely.
> >>
> >> So how is this dealt with for usbip ?
> > 
> > As far as I can tell, it isn't. Running a storage device over usbip
> > is a bit dangerous.
> 
> I don't follow that analysis.  The usbip interactions with the usb stack
> all seem to be atomic, and never trigger a syscall, as far as I can
> tell.  A port reset will flip a few bits and return.  A urb enqueue
> queues and wakes a different thread, and returns.  The alternate thread
> performs the sendmsg.
> 
> I'm not suggesting that running a storage device over usbip is
> especially safe, but I don't see the limit on the design.

Are you referring to the current code or the proposed user space pipe?

Regards
Oliver



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

Re: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Oliver Neukum]

On Thu, 2015-07-02 at 13:35 +0200, Hans de Goede wrote:
> Hi,
> 
> On 02-07-15 10:45, Oliver Neukum wrote:
> > On Wed, 2015-07-01 at 10:06 +0100, Daniel P. Berrange wrote:
> >
> >> I don't really think it is sensible to be defining & implementing new
> >> network services which can't support strong encryption and authentication.
> >> Rather than passing the file descriptor to the kernel and having it do
> >> the I/O directly, I think it would be better to dissassociate the kernel
> >> from the network transport, and thus leave all sockets layer data I/O
> >> to userspace daemons so they can layer in TLS or SASL or whatever else
> >> is appropriate for the security need.
> >
> > Hi,
> >
> > this hits a fundamental limit. Block IO must be done entirely in kernel
> > space or the system will deadlock. The USB stack is part of the block
> > layer and the SCSI error handling. Thus if you involve user space you
> > cannot honor memory allocation with GFP_NOFS and you break all APIs
> > where we pass GFP_NOIO in the USB stack.
> >
> > Supposed you need to reset a storage device for error handling.
> > Your user space programm does a syscall, which allocates memory
> > and needs to launder pages. It proceeds to write to the storage device
> > you wish to reset.
> >
> > It is the same problem FUSE has with writable mmap. You cannot do
> > block devices in user space sanely.
> 
> So how is this dealt with for usbip ?

As far as I can tell, it isn't. Running a storage device over usbip
is a bit dangerous.

Regards
Oliver


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

Re: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Oliver Neukum]

On Wed, 2015-07-01 at 10:06 +0100, Daniel P. Berrange wrote:

> I don't really think it is sensible to be defining & implementing new
> network services which can't support strong encryption and authentication.
> Rather than passing the file descriptor to the kernel and having it do
> the I/O directly, I think it would be better to dissassociate the kernel
> from the network transport, and thus leave all sockets layer data I/O
> to userspace daemons so they can layer in TLS or SASL or whatever else
> is appropriate for the security need.

Hi,

this hits a fundamental limit. Block IO must be done entirely in kernel
space or the system will deadlock. The USB stack is part of the block
layer and the SCSI error handling. Thus if you involve user space you
cannot honor memory allocation with GFP_NOFS and you break all APIs
where we pass GFP_NOIO in the USB stack.

Supposed you need to reset a storage device for error handling.
Your user space programm does a syscall, which allocates memory
and needs to launder pages. It proceeds to write to the storage device
you wish to reset.

It is the same problem FUSE has with writable mmap. You cannot do
block devices in user space sanely.

Sorry
Oliver


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

Re: [PATCH] usb: move assignment out of if condition

[Oliver Neukum]

On Tue, 2015-06-30 at 09:02 -0400, Kris Borer wrote:
> Fix four occurrences of the checkpatch.pl error:
> 
> ERROR: do not use assignment in if condition
> 
> Signed-off-by: Kris Borer 

Sorry, but NACK!
Those patches are totally broken.

> ---
>  drivers/usb/core/hcd.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index be5b207..e268c45 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2683,12 +2683,13 @@ int usb_add_hcd(struct usb_hcd *hcd,
>* bottom up so that hcds can customize the root hubs before hub_wq
>* starts talking to them.  (Note, bus id is assigned early too.)
>*/
> - if ((retval = hcd_buffer_create(hcd)) != 0) {
> + retval = hcd_buffer_create(hcd);
> + if (retval != 0) {
>   dev_dbg(hcd->self.controller, "pool alloc failed\n");
>   goto err_create_buf;
>   }
> -
> - if ((retval = usb_register_bus(&hcd->self)) < 0)
> + retval = usb_register_bus(&hcd->self);
> + if (retval < 0)
>   goto err_register_bus;
>  
>   rhdev = usb_alloc_dev(NULL, &hcd->self, 0);
> @@ -2734,7 +2735,8 @@ int usb_add_hcd(struct usb_hcd *hcd,
>   /* "reset" is misnamed; its role is now one-time init. the controller
>* should already have been reset (and boot firmware kicked off etc).
>*/
> - if (hcd->driver->reset && (retval = hcd->driver->reset(hcd)) < 0) {
> + retval = hcd->driver->reset(hcd);

You happily follow the NULL pointer.

Regards
Oliver


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

Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices

[Oliver Neukum]

On Mon, 2015-06-29 at 13:16 +0200, Jiri Kosina wrote:
> On Mon, 29 Jun 2015, Oliver Neukum wrote:
> 
> > > Last time we were testing this, autosuspend for USB HID devices was quite 
> > > a disaster.
> > > 
> > > Do you have any idea whether udev developers tested the "autosuspend on 
> > > by 
> > > default for USB HID devices" on reasonable set of devices?
> > > 
> > > The culrpits that I remember from top of my head (it's been long time 
> > > ago):
> > > 
> > > - the LEDs for suspended device go off. This is very confusing at least 
> > > on 
> > >   keyboards, and brings really bad user experience
> > 
> > That is a bug. hidinput_count_leds() is supposed to prevent that.
> 
> This is a HW property and nothing kernel can do about. I am not saying it 
> doesn't bring the LEDs up to a proper state again once auto-resumed. But I 
> hate the LEDs going off a few seconds after I stop typing (i.e. once the 
> keyboard gets auto-suspended).

That is the point. Unless you give the option to override, they
shouldn't autosuspend.

Regards
Oliver



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

Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices

[Oliver Neukum]

On Sat, 2015-06-27 at 08:29 +0200, Jiri Kosina wrote:
> On Fri, 26 Jun 2015, Greg Kroah-Hartman wrote:

> Last time we were testing this, autosuspend for USB HID devices was quite 
> a disaster.
> 
> Do you have any idea whether udev developers tested the "autosuspend on by 
> default for USB HID devices" on reasonable set of devices?
> 
> The culrpits that I remember from top of my head (it's been long time 
> ago):
> 
> - the LEDs for suspended device go off. This is very confusing at least on 
>   keyboards, and brings really bad user experience

That is a bug. hidinput_count_leds() is supposed to prevent that.
What did you test?

> - many keyboards were losing first keystroke when waking up from suspend. 
>   We've been debugging this with Alan, and never root-caused it to a 
>   problem in our code, it seems to be the property of the HW

- mice don't wake up from movement alone.

And again I would state that we don't get enough information
from user space. Hardware seems to be designed for sleeping
while a screen saver is on. In kernel space we just get a binary
input desired or not desired from open/close.
That is insufficient.

Regards
Oliver



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

Re: [PATCH 1/1] cdc-acm: Add support of ATOL FPrint fiscal printers

[Oliver Neukum]

On Tue, 2015-06-02 at 11:05 +0300, Alexey Sokolov wrote:
> ATOL FPrint fiscal printers require usb_clear_halt to be executed
> to work properly. Add quirk to fix the issue.
> 


Hi,

thank you for this patch.

> Signed-off-by: Alexey Sokolov 
> ---
>  drivers/usb/class/cdc-acm.c | 9 +
>  drivers/usb/class/cdc-acm.h | 1 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index 5c8f581..9d8a321 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -1477,6 +1477,11 @@ skip_countries:
>   goto alloc_fail8;
>   }
>  
> + if (quirks == CLEAR_HALT_CONDITIONS) {

Please make that
quirks & CLEAR_HALT_CONDITIONS

Regards
Oliver

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

Re: [PATCH 1/2] Bluetooth: Add reset_resume function

[Oliver Neukum]

On Mon, 2015-06-01 at 18:14 -0700, Laura Abbott wrote:
> Bluetooth devices off of some buses such as USB may lose power across
> suspend/resume. When this happens, drivers may need to have the setup
> function called again and behave differently than a cold power on.

Yes, but what is the point? We use reset_resume() to retain
some features of a device across a loss of power.
If power is lost, all settings are gone and all connections
are broken. So what is the difference compared to a plug out/in
cycle?

Regards
Oliver



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

Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable

[Oliver Neukum]

On Thu, 2015-05-21 at 22:46 +0200, Arend van Spriel wrote:
> On 05/21/15 19:32, Takashi Iwai wrote:


> > Well, if the probe requires the access to a user-space file, it can't
> > be done during resume.  That's the very problem we're seeing now.
> > The firmware loader can't help much alone if it's a new device
> > object.
> 
> So you are saying each device driver should come up with some retry 
> mechanism. Would make more sense to come up with something like that 
> behind the scenes in the firmware loader so all device drivers can rely 
> on one and the same solution.

There is already a notifier for this. I don't see why the firmware
layer couldn't retrigger a match for all unbound devices, just like
we do when a new driver is loaded.

Regards
Oliver


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

Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable

[Oliver Neukum]

On Wed, 2015-05-20 at 08:29 +0200, Takashi Iwai wrote:
> The data is cached in RAM.  More specifically, the former loaded
> firmware files are reloaded and saved at suspend for each device
> object.  See fw_pm_notify() in firmware_class.c.

OK, this may be a stupid idea, but do we know the firmware
was successfully loaded in the first place?
Also btusb is in the habit of falling back to a generic
firmware in some places. It seems to me that caching
firmware is conceptually not enough, but we'd also need
to record the absence of firmware images.

Regards
Oliver


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

Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable

[Oliver Neukum]

On Tue, 2015-05-19 at 19:13 +0200, Takashi Iwai wrote:
> At Tue, 19 May 2015 10:26:46 -0400 (EDT),
> Alan Stern wrote:
> > 
> > > > >  Of just have request_firmware()
> > > > > actually sleep until userspace is ready. Seriously, why is
> > > > > request_firmware not just sleeping for us.
> > 
> > It won't work.  The request_firmware call is part of the probe 
> > sequence, which in turn is part of the resume sequence.  Userspace 
> > doesn't start running again until the resume sequence is finished.  If 
> > request_firmware waited for userspace, it would hang.
> 
> Note that the recent request_firmware() doesn't need the user-space
> invocation (unless the fallback is explicitly enabled) but loads the

That is a dangerous approach. You cannot be sure you can do file IO.
It depends on the exact shape of the device tree.

> file directly.  And, request_firmware() for the cached data is valid
> to be called in the resume path.

Well, yes, if your data is cached in RAM, all is well. But that leads
to the same problem one step further. What must be cached?

Regards
Oliver



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

Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable

[Oliver Neukum]

On Tue, 2015-05-19 at 10:26 -0400, Alan Stern wrote:
> On Tue, 19 May 2015, Takashi Iwai wrote:
> 
> > > > I am not convinced. Now we are hacking the Bluetooth core layer
> > > > (which has nothing to do with the drivers suspend/resume or
> > > > probe) to do something different so that we do not see this
> > > > warning.
> > > >
> > > > I can not do anything about the platform in question choosing a
> > > > unplug/replug for suspend/resume instead of having a proper USB
> > > > suspend and resume handling. That is pretty much out of our
> > > > control.
> 
> Actually one can do something about this.  I mean, one _can_ implement
> proper USB suspend and resume handling in the Bluetooth driver.  At
> this point the details aren't clear to me, but perhaps if the driver in
> question had a reset_resume callback then it might work better.

I doubt this would work. By losing power the BT controller is thrown
out of its cell. It looks to me like fundamentally BT needs to
fully reestablish the network from scratch after a loss of power.

> > > >  I would rather have the USB subsystem delay the probe()
> > > > callback if we tell it to.
> 
> This is possible.  I am not sure it would be the right thing to do,
> though.  What happens if the probe routine gets called early on during
> the boot-up procedure, before userspace is up and running?  The same
> thing should happen here.

Yes. Basically if you want firmware during probe the firmware
infrastructure has to be there. That is if you build such a module
statically the firmware must be included in the kernel image.

> > > >  Of just have request_firmware()
> > > > actually sleep until userspace is ready. Seriously, why is
> > > > request_firmware not just sleeping for us.
> 
> It won't work.  The request_firmware call is part of the probe 
> sequence, which in turn is part of the resume sequence.  Userspace 
> doesn't start running again until the resume sequence is finished.  If 
> request_firmware waited for userspace, it would hang.

I'd recommend the sledge hammer. Never free the firmware while the
hardware is connected or the system sleeping. If you must do this
there is a notifier chain.

Regards
Oliver



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

Re: [PATCH 1/1 linux-next] cdc-acm: use swap() in acm_probe()

[Oliver Neukum]

On Mon, 2015-05-18 at 19:59 +0200, Fabian Frederick wrote:
> Use kernel.h macro definition.
> 
> Signed-off-by: Fabian Frederick 
Acked-by: Oliver Neukum 


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

Re: [PATCH] HID: usbhid: Add HID_QUIRK_NOGET for Aten DVI KVM switch

[Oliver Neukum]

On Wed, 2015-05-13 at 13:25 +0200, Jiri Kosina wrote:
> On Tue, 12 May 2015, Laura Abbott wrote:
> 
> > Like other KVM switches, the Aten DVI KVM switch needs a
> > quirk to avoid spewing errors:
> > 
> > [791759.606542] usb 1-5.4: input irq status -75 received
> > [791759.614537] usb 1-5.4: input irq status -75 received
> > [791759.622542] usb 1-5.4: input irq status -75 received
> > 
> > Add it.
> > 
> > Signed-off-by: Laura Abbott 
> 
> Applied for for-4.1/upstream-fixes, thanks.

Hi,

this should go into stable, too.

Regards
Oliver



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

Re: [PATCH 1/1] suspend: delete sys_sync()

[Oliver Neukum]

On Mon, 2015-05-11 at 16:34 -0400, Len Brown wrote:
> But upon reflection, I do not believe that the kernel is adding value
> here,
> instead it is imposing a policy, and that policy decision is sometimes
> prohibitively expensive.
> User-space can do this for itself (and in the case of desktop distros,
> already does),
> and so the kernel should butt-out.

There is however a race condition with doing it in user space.
So adding a parameter to select behavior would increase safety a bit.

Regards
Oliver


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

Re: [PATCH] net: usb: allow MTU that is a multiple of USB packet size

[Oliver Neukum]

On Thu, 2015-05-07 at 13:51 +0300, Ruslan Bilovol wrote:
> Hi Oliver,
> 
> On Thu, May 7, 2015 at 1:11 PM, Oliver Neukum  wrote:
> > On Thu, 2015-05-07 at 12:49 +0300, Ruslan Bilovol wrote:
> >> Current usbnet driver rejects setting MTU that is a multiple
> >> of USB endpoint's wMaxPacketSize size. However, it may only
> >> lead to possible performance degradation but is not so
> >> critical that its using should be prohibited. So allow it
> >> but also warn user about possible issue.
> >
> > We have reports about devices reacting badly to ZLPs.
> > Unless you have a compelling reasons for this change
> > I have to reject it.
> 
> What devices do you mean here: USB network adapters or USB host controllers?

The network adapters

> If it's just network adapters, then we can create some kind of quirk handling
> for them and do not disable this functionality just because some buggy device
> can't work with it.

No. They are too many.

> My device works fine with ZLPs so I want to use this particular MTU under 
> Linux
> like I do it under other operation systems.

We can have a white list, but the general case is just too dangerous.

Sorry
Oliver


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

Re: [PATCH] net: usb: allow MTU that is a multiple of USB packet size

[Oliver Neukum]

On Thu, 2015-05-07 at 12:49 +0300, Ruslan Bilovol wrote:
> Current usbnet driver rejects setting MTU that is a multiple
> of USB endpoint's wMaxPacketSize size. However, it may only
> lead to possible performance degradation but is not so
> critical that its using should be prohibited. So allow it
> but also warn user about possible issue.

We have reports about devices reacting badly to ZLPs.
Unless you have a compelling reasons for this change
I have to reject it.

Regards
Oliver

NACK

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

Re: [PATCH] USB: usbmon: Use 64bit timestamp for mon_bin_hdr

[Oliver Neukum]

On Tue, 2015-05-05 at 11:50 +0530, Tina Ruchandani wrote:
> struct mon_bin_hdr allows for a 64-bit seconds timestamp. The code 
> currently uses 'struct timeval' to populate the timestamp in mon_bin_hdr, 
> which has a 32-bit seconds field and will overflow in year 2038 and beyond.
> This patch replaces 'struct timeval' with 'struct timespec64' which is 
> y2038 safe.

Hi,

but the timestamp will also overflow. So what is the point?

Regards
Oliver


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

Re: [PATCH 6/6] USB / PM: Allow USB devices to remain runtime-suspended when sleeping

[Oliver Neukum]

On Wed, 2015-04-01 at 10:01 -0400, Alan Stern wrote:
> On Wed, 1 Apr 2015, Oliver Neukum wrote:
> 
> > On Tue, 2015-03-31 at 13:09 -0400, Alan Stern wrote:
> > > In other words, if the device is currently in runtime suspend with 
> > > remote wakeup enabled, but device_may_wakeup() returns 0 (so that the 
> > > device should be disabled for wakeup when the system goes into 
> > > suspend), then the prepare callback has to return 0.
> > > 
> > > Therefore what you need to do here is something like this:
> > > 
> > > struct usb_device   *udev = to_usb_device(dev);
> > > 
> > > /* Return 0 if the current wakeup setting is wrong, otherwise
> > > 1 */
> > 
> > And the other way round?
> 
> Your meaning isn't clear.  Are you asking what should happen if the 
> device is in runtime suspend with remote wakeup disabled, but 
> device_may_wakeup() returns 1?

Yes. I was thinking about that case.

>   That case should never happen -- but 
> if it does then the prepare callback should return 0.

Exactly. It didn't seem to do so.

Regards
Oliver





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

Re: [PATCH 6/6] USB / PM: Allow USB devices to remain runtime-suspended when sleeping

[Oliver Neukum]

On Tue, 2015-03-31 at 13:09 -0400, Alan Stern wrote:
> In other words, if the device is currently in runtime suspend with 
> remote wakeup enabled, but device_may_wakeup() returns 0 (so that the 
> device should be disabled for wakeup when the system goes into 
> suspend), then the prepare callback has to return 0.
> 
> Therefore what you need to do here is something like this:
> 
> struct usb_device   *udev = to_usb_device(dev);
> 
> /* Return 0 if the current wakeup setting is wrong, otherwise
> 1 */

And the other way round?

Regards
Oliver



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

Re: [PATCH] usb: Enable LPM for USB 2.01+ full-speed devices

[Oliver Neukum]

On Wed, 2015-03-25 at 12:23 +0530, rtat...@codeaurora.org wrote:
> From: Rupesh Tatiya 
> 
> USB 2.01+ full-speed devices can have extended descriptor as well
> and can support LPM.

Yes, they in theory can, but what happens if they are actually
asked to do so? On how many devices have you tested this patch?

Regards
Oliver


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

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Oliver Neukum]

On Thu, 2015-03-19 at 11:12 +0100, Pavel Machek wrote:
> On Thu 2015-03-19 10:54:22, Oliver Neukum wrote:
> > On Thu, 2015-03-19 at 10:38 +0100, Pavel Machek wrote:
> > > On Thu 2015-03-19 10:14:21, Oliver Neukum wrote:
> > > > On Mon, 2015-03-16 at 22:37 +0100, Jiri Kosina wrote:
> > 
> > > > > Are you sure CONFIG_DMA_API_DEBUG wouldn't warn here?
> > > > 
> > > > As far as I can tell, it will not warn. The problem is not in the
> > > > mapping itself. That is usually legitimate. The problem arises
> > > > because the buffer doesn't have a cacheline of its own. Thus the
> > > > memory corruption happens after the IO operation has started.
> > > 
> > > Nasty. Would WARN_ON(buffer & CACHELINE_SIZE-1) do at least part of
> > 
> > No. It is perfectly legitimate to put your buffer at an offset
> > or to combine buffers provided you don't use them at the same
> > time.
> 
> Legitimate: yes. Is anyone doing it? And will not they see exactly the

For this particular function probably not. In the general case, yes.
That's how you continue after an error.

> same data corruption with the aliasing data?

No, the error happens by touching another part of the cacheline
from the CPU thus loading stale content into the cache.
You can even have simultaneous DMA to two buffers in the same cacheline
provided you touch neither until the last DMA has finished.

> > > Alternatively, we could create "allocate_for_usb" function, and only
> > > take pointers allocated by that function in usb functions. That would
> > > also teach people the problem exists...
> > 
> > No, this problem is not limited to USB.
> 
> Well.. Recognize that just because you have a pointer does not mean
> you can pass it to certain functions.
> 
> Maybe those functions should not be taking pointers in the first
> place

What else would it take? Should we force people to allocate a new
buffer every time? That would make the API for reading and writing
asymmetric.

Regards
Oliver


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

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Oliver Neukum]

On Thu, 2015-03-19 at 10:38 +0100, Pavel Machek wrote:
> On Thu 2015-03-19 10:14:21, Oliver Neukum wrote:
> > On Mon, 2015-03-16 at 22:37 +0100, Jiri Kosina wrote:

> > > Are you sure CONFIG_DMA_API_DEBUG wouldn't warn here?
> > 
> > As far as I can tell, it will not warn. The problem is not in the
> > mapping itself. That is usually legitimate. The problem arises
> > because the buffer doesn't have a cacheline of its own. Thus the
> > memory corruption happens after the IO operation has started.
> 
> Nasty. Would WARN_ON(buffer & CACHELINE_SIZE-1) do at least part of

No. It is perfectly legitimate to put your buffer at an offset
or to combine buffers provided you don't use them at the same
time.

> the trick? Alternatively, could we call ksize() on the object, and
> fail if it is not big enough?

What object? We have a pointer to a memory location.

> Alternatively, we could create "allocate_for_usb" function, and only
> take pointers allocated by that function in usb functions. That would
> also teach people the problem exists...

No, this problem is not limited to USB.

Regards
Oliver


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

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Oliver Neukum]

On Mon, 2015-03-16 at 22:37 +0100, Jiri Kosina wrote:
> On Mon, 16 Mar 2015, Pavel Machek wrote:
> 
> > > > Oliver Neukum  wrote:
> > > > 
> > > > > > +   ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02),
> > > > > > +   buf2, sizeof(buf2),
> > > > > > +   &transfered, USB_CTRL_SET_TIMEOUT);
> > > > > 
> > > > > You cannot do this. Even for a single byte DMA on the stack is
> > > > > wrong. Not on all architectures it works at all and you violate
> > > > > the DMA constrainsts. You must use kmalloc().
> > > > 
> > > > Hi Oliver,
> > > > 
> > > > Does this still apply when using hid_hw_output_report?
> > > 
> > > Yes. For USB devices hid_hw_output_report() goes to
> > > usbhid_output_report(). That goes to usb_interrupt_msg(),
> > > which passes the buffer pointer. It will then be mapped
> > > for DMA. You must not do that on the stack.
> > 
> > Should we have some kind of runtime test for this ...? Because this is
> > very very easy to get wrong... and I bet we do get it wrong at > 1
> > place...
> 
> Are you sure CONFIG_DMA_API_DEBUG wouldn't warn here?

As far as I can tell, it will not warn. The problem is not in the
mapping itself. That is usually legitimate. The problem arises
because the buffer doesn't have a cacheline of its own. Thus the
memory corruption happens after the IO operation has started.

Regards
Oliver



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

Re: [RFC 2/4] power: core: Add generic interface to get battery specification.

[Oliver Neukum]

int power_supply_reg_notifier(struct notifier_block *nb);
>  extern void power_supply_unreg_notifier(struct notifier_block *nb);
> @@ -311,6 +316,13 @@ extern int power_supply_powers(struct power_supply *psy, 
> struct device *dev);
>  /* For APM emulation, think legacy userspace. */
>  extern struct class *power_supply_class;
>  
> +/* Battery information helper */
> +extern int psy_battery_info_notifier_register(struct notifier_block *);
> +extern void psy_battery_info_notifier_unregister(struct notifier_block *);
> +extern struct psy_charging_obj *psy_get_battery_info(const char *);
> +extern int psy_register_battery_info(struct psy_charging_obj *);
> +extern void psy_unregister_battery_info(struct psy_charging_obj *);
> +
>  static inline bool power_supply_is_amp_property(enum power_supply_property 
> psp)
>  {
>   switch (psp) {


-- 
Oliver Neukum 

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

Re: [RFC 1/4] power_supply: Introduce charging object table

[Oliver Neukum]

On Fri, 2015-03-06 at 16:03 +0530, Jenny TC wrote:
> Charging current (CC) and charging voltage (CV) may vary based on
> battery temperature. To support CC and CV for different temperature
> zones, defined a charging object which holds the properties related
> to battery charging.
> 
> Signed-off-by: Jenny TC 
> ---
>  include/linux/power_supply.h |   27 +++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 096dbce..7aada44 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -252,6 +252,33 @@ struct power_supply_info {
>   int use_for_apm;
>  };
>  
> +
> +struct psy_temp_mon_table {
> + int temp_max;
> + int temp_min;
> + int charging_current; /* CC */
> + int charging_voltage; /* CV */

In which units?

> + /* delta voltage at which charging should restart */
> + int maint_voltage_delta;
> +};
> +
> +#define PSY_MAX_BAT_NAME_LEN 8
> +#define PSY_MAX_TEMP_ZONE 6
> +
> +struct psy_charging_obj {
> + char name[PSY_MAX_BAT_NAME_LEN];
> + int battery_type;
> + int temp_max;
> + int temp_min;
> + int full_condition_soc;
> + int full_condition_capacity;
> + int full_condition_voltage;
> + int iterm; /* charge termination current */
> + /* CC/CV table for different temperature range */
> + int temp_mon_count; /* number of entries in temp_mon_table */
> + struct psy_temp_mon_table temp_mon_table[PSY_MAX_TEMP_ZONE];
> +};
> +
>  extern struct atomic_notifier_head power_supply_notifier;
>  extern int power_supply_reg_notifier(struct notifier_block *nb);
>  extern void power_supply_unreg_notifier(struct notifier_block *nb);


-- 
Oliver Neukum 

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

Re: [PATCH 1/9] HSI: cmt_speech: Add cmt-speech driver

[Oliver Neukum]

> +static ssize_t cs_char_read(struct file *file, char __user *buf, size_t 
> count,
> + loff_t *unused)
> +{
> + struct cs_char *csdata = file->private_data;
> + u32 data;
> + ssize_t retval;
> +
> + if (count < sizeof(data))
> + return -EINVAL;
> +
> + for ( ; ; ) {
> + DEFINE_WAIT(wait);
> +
> + spin_lock_bh(&csdata->lock);
> + if (!list_empty(&csdata->chardev_queue)) {
> + data = cs_pop_entry(&csdata->chardev_queue);
> + } else if (!list_empty(&csdata->dataind_queue)) {
> + data = cs_pop_entry(&csdata->dataind_queue);
> + --csdata->dataind_pending;
> +
> + } else {
> + data = 0;
> + }
> + spin_unlock_bh(&csdata->lock);
> +
> + if (data)
> + break;
> + if (file->f_flags & O_NONBLOCK) {
> + retval = -EAGAIN;
> + goto out;
> + } else if (signal_pending(current)) {
> + retval = -ERESTARTSYS;
> + goto out;
> + }
> + prepare_to_wait_exclusive(&csdata->wait, &wait,
> + TASK_INTERRUPTIBLE);
> + schedule();
> + finish_wait(&csdata->wait, &wait);
> + }
> +
> + retval = put_user(data, (u32 __user *)buf);
> + if (!retval)
> + retval = sizeof(data);
> +
> +out:
> + return retval;
> +}
> +
> +static ssize_t cs_char_write(struct file *file, const char __user *buf,
> + size_t count, loff_t *unused)
> +{
> + struct cs_char *csdata = file->private_data;
> + u32 data;
> + int err;
> + ssize_t retval;
> +
> + if (count < sizeof(data))
> + return -EINVAL;
> +
> + if (get_user(data, (u32 __user *)buf))
> + retval = -EFAULT;
> + else
> + retval = count;

You want to execute the command even if you got -EFAULT?
That is highly unusual.

> +
> + err = cs_hsi_command(csdata->hi, data);
> + if (err < 0)
> + retval = err;
> +
> + return retval;
> +}
> +
> +static long cs_char_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct cs_char *csdata = file->private_data;
> + int r = 0;
> +
> + switch (cmd) {
> + case CS_GET_STATE: {
> + unsigned int state;
> +
> + state = cs_hsi_get_state(csdata->hi);
> + if (copy_to_user((void __user *)arg, &state, sizeof(state)))
> + r = -EFAULT;
> + }
> + break;
> + case CS_SET_WAKELINE: {
> + unsigned int state;
> +
> + if (copy_from_user(&state, (void __user *)arg, sizeof(state)))
> + r = -EFAULT;
> + else
> + cs_hsi_set_wakeline(csdata->hi, state);

No sanity checking for state?

> + }
> + break;
> + case CS_GET_IF_VERSION: {
> + unsigned int ifver = CS_IF_VERSION;
> +
> + if (copy_to_user((void __user *)arg, &ifver, sizeof(ifver)))
> + r = -EFAULT;
> + break;
> + }
> + case CS_CONFIG_BUFS: {
> + struct cs_buffer_config buf_cfg;
> +
> + if (copy_from_user(&buf_cfg, (void __user *)arg,
> + sizeof(buf_cfg)))
> + r = -EFAULT;
> + else
> + r = cs_hsi_buf_config(csdata->hi, &buf_cfg);

Sanity checking?

> + break;
> + }
> + default:
> + r = -ENOTTY;
> + break;
> + }
> +
> + return r;
> +}
> +
> +static int cs_char_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> + if (vma->vm_end < vma->vm_start)
> + return -EINVAL;
> +
> + if (((vma->vm_end - vma->vm_start) >> PAGE_SHIFT) != 1)
> + return -EINVAL;
> +
> + vma->vm_flags |= VM_RESERVED;
> + vma->vm_ops = &cs_char_vm_ops;
> + vma->vm_private_data = file->private_data;
> +
> + return 0;
> +}
> +
> +static int cs_char_open(struct inode *unused, struct file *file)
> +{
> + int ret = 0;
> +
> + spin_lock_bh(&cs_char_data.lock);
> + if (cs_char_data.opened) {
> + ret = -EBUSY;
> + spin_unlock_bh(&cs_char_data.lock);
> + goto out;
> + }
> + cs_char_data.mmap_base = get_zeroed_page(GFP_ATOMIC);

This could be moved outside the locked sectionand use GFP_KERNEL.

> + if (!cs_char_data.mmap_base) {
> + dev_err(&cs_char_data.cl->device,
> + "Shared memory allocation failed.\n");
> + ret = -ENOMEM;
> + spin_unlock_bh(&cs_char_data.lock);
> + goto out;
> + 

Re: [PATCH 1/9] HSI: cmt_speech: Add cmt-speech driver

[Oliver Neukum]

On Mon, 2015-03-02 at 05:38 +0100, Sebastian Reichel wrote:
> +static int cs_alloc_cmds(struct cs_hsi_iface *hi)
> +{
> +   struct hsi_msg *msg;
> +   u32 *buf;
> +   unsigned int i;
> +
> +   INIT_LIST_HEAD(&hi->cmdqueue);
> +
> +   for (i = 0; i < CS_MAX_CMDS; i++) {
> +   msg = hsi_alloc_msg(1, GFP_ATOMIC);

Why does this need to be ATOMIC?
> +   if (!msg)
> +   goto out;
> +   buf = kmalloc(sizeof(*buf), GFP_ATOMIC);
> +   if (!buf) {
> +   hsi_free_msg(msg);
> +   goto out;
> +   }
> +   sg_init_one(msg->sgt.sgl, buf, sizeof(*buf));
> +   msg->channel = CONTROL_HSI_CH;
> +   msg->context = hi;
> +   list_add_tail(&msg->link, &hi->cmdqueue);
> +   }
> +
> +   return 0;
> +
> +out:
> +   cs_free_cmds(hi);
> +   return -ENOMEM;
> +}
> +



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

Re: [PATCH v11 2/6] backlight: Add support Skyworks SKY81452 backlight driver

[Oliver Neukum]

On Fri, 2015-02-27 at 15:42 +0900, gyun...@gmail.com wrote:
> +static ssize_t sky81452_bl_store_enable(struct device *dev,
> +   struct device_attribute *attr, const char *buf, size_t
> count)
> +{
> +   struct regmap *regmap = bl_get_data(to_backlight_device(dev));
> +   unsigned long value;
> +   int ret;
> +
> +   ret = kstrtoul(buf, 16, &value);
> +   if (IS_ERR_VALUE(ret))
> +   return ret;

No range checking?

> +
> +   ret = regmap_update_bits(regmap, SKY81452_REG1, SKY81452_EN,
> +   value << CTZ(SKY81452_EN));
> +   if (IS_ERR_VALUE(ret))
> +   return ret;
> +
> +   return count;
> +}

Regards
Oliver



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

Re: [PATCH 2/4] usb: Introduce Xen pvUSB frontend

[Oliver Neukum]

On Thu, 2015-02-26 at 14:35 +0100, Juergen Gross wrote:
> +
> +   /* reset completion */
> +   if ((info->ports[wIndex].status &
> USB_PORT_STAT_RESET) != 0 &&
> +   time_after_eq(jiffies,
> info->ports[wIndex].timeout)) {
> +   info->ports[wIndex].status |=
> +   USB_PORT_STAT_C_RESET << 16;
> +   info->ports[wIndex].status &=
> ~USB_PORT_STAT_RESET;
> +
> +   if (info->devices[wIndex].status !=
> +   USB_STATE_NOTATTACHED) {
> +   info->ports[wIndex].status |=
> +   USB_PORT_STAT_ENABLE;
> +   info->devices[wIndex].status =
> +   USB_STATE_DEFAULT;
> +   }
> +
> +   switch (info->devices[wIndex].speed) {
> +   case USB_SPEED_LOW:
> +   info->ports[wIndex].status |=
> +   USB_PORT_STAT_LOW_SPEED;
> +   break;
> +   case USB_SPEED_HIGH:
> +   info->ports[wIndex].status |=
> +   USB_PORT_STAT_HIGH_SPEED;
> +   break;
> +   default:
> +   break;
> +   }
> +   }
> +
> +   ((u16 *)buf)[0] =
> cpu_to_le16(info->ports[wIndex].status);
> +   ((u16 *)buf)[1] =
> cpu_to_le16(info->ports[wIndex].status >> 16);

Why in two chunks?
Regards
Oliver
> +   break;


-- 
Oliver Neukum 

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

Re: [PATCH 2/2] INPUT/HID: add touch support for SiS touch driver

[Oliver Neukum]

tl*/
> +static int sis_setup_chardev(struct hid_device *hdev) {
> +  dev_t dev = MKDEV(sis_char_major, 0);
> +  int ret = 0;
> +  struct device *class_dev = NULL;
> +  u8 *name = (hdev->product == USB_DEVICE_ID_SISF817_TOUCH) ?
> +  SISF817_DEVICE_NAME : SIS817_DEVICE_NAME;
> +
> +  dev_info(&hdev->dev, "sis_setup_chardev.\n");
> +
> +  backup_urb = usb_alloc_urb(0, GFP_KERNEL);/*0721test*/
> +  if (!backup_urb) {
> +  dev_err(&hdev->dev, "cannot allocate backup_urb\n");
> +  return -ENOMEM;
> +  }
> +  hid_dev_backup = hdev;
> +  /*dynamic allocate driver handle*/
> +  ret = alloc_chrdev_region(&dev, 0, sis_char_devs_count, name);
> +  if (ret)
> +  goto err1;
> +
> +  sis_char_major = MAJOR(dev);
> +  cdev_init(&sis_char_cdev, &sis_cdev_fops);
> +  sis_char_cdev.owner = THIS_MODULE;
> +  ret = cdev_add(&sis_char_cdev, dev, sis_char_devs_count);
> +  if (ret)
> +  goto err2;
> +
> +  dev_info(&hdev->dev, "%s driver(major %d) installed.\n",
> +  name, sis_char_major);
> +
> +  /*register class*/
> +  sis_char_class = class_create(THIS_MODULE, name);
> +  if (IS_ERR(sis_char_class)) {
> +  ret = PTR_ERR(sis_char_class);
> +  goto err3;
> +  }
> +
> +  class_dev = device_create(sis_char_class, NULL, dev, NULL, name);
> +  if (IS_ERR(class_dev)) {
> +  ret = PTR_ERR(class_dev);
> +  goto err4;
> +  }
> +
> +  return 0;
> +err4:
> +  class_destroy(sis_char_class);
> +  sis_char_class = NULL;
> +err3:
> +  cdev_del(&sis_char_cdev);
> +  memset(&sis_char_cdev, 0, sizeof(struct cdev));
> +err2:
> +  sis_char_major = 0;
> +  unregister_chrdev_region(dev, sis_char_devs_count);
> +err1:
> +  usb_kill_urb(backup_urb);
> +  usb_free_urb(backup_urb);
> +  backup_urb = NULL;
> +  hid_dev_backup = NULL;
> +  return ret;
> +}
> +
> +/*for ioctl*/
> +static void sis_deinit_chardev(void)
> +{
> +  dev_t dev = MKDEV(sis_char_major, 0);
> +
> +  if (hid_dev_backup) {
> +  dev_info(&hid_dev_backup->dev, "sis_remove\n");
> +  device_destroy(sis_char_class, dev);
> +  class_destroy(sis_char_class);
> +  sis_char_class = NULL;
> +  cdev_del(&sis_char_cdev);
> +  memset(&sis_char_cdev, 0, sizeof(struct cdev));
> +  sis_char_major = 0;
> +  unregister_chrdev_region(dev, sis_char_devs_count);
> +  usb_kill_urb(backup_urb);
> +  usb_free_urb(backup_urb);
> +  backup_urb = NULL;
> +  hid_dev_backup = NULL;
> +  }
> +}
> diff --git a/linux-3.18.5/drivers/hid/usbhid/hid-quirks.c 
> b/linux-3.18.5/drivers/hid/usbhid/hid-quirks.c
> index 4477eb7..b534321 100644
> --- a/linux-3.18.5/drivers/hid/usbhid/hid-quirks.c
> +++ b/linux-3.18.5/drivers/hid/usbhid/hid-quirks.c
> @@ -97,6 +97,7 @@ static const struct hid_blacklist {
>   { USB_VENDOR_ID_SIGMATEL, USB_DEVICE_ID_SIGMATEL_STMP3780, 
> HID_QUIRK_NOGET },
>   { USB_VENDOR_ID_SIS_TOUCH, USB_DEVICE_ID_SIS9200_TOUCH, HID_QUIRK_NOGET 
> },
>   { USB_VENDOR_ID_SIS_TOUCH, USB_DEVICE_ID_SIS817_TOUCH, HID_QUIRK_NOGET 
> },
> +  { USB_VENDOR_ID_SIS_TOUCH, USB_DEVICE_ID_SISF817_TOUCH, 
> + HID_QUIRK_NOGET },
>   { USB_VENDOR_ID_SIS_TOUCH, USB_DEVICE_ID_SIS_TS, 
> HID_QUIRK_NO_INIT_REPORTS },
>   { USB_VENDOR_ID_SIS_TOUCH, USB_DEVICE_ID_SIS1030_TOUCH, HID_QUIRK_NOGET 
> },
>   { USB_VENDOR_ID_SUN, USB_DEVICE_ID_RARITAN_KVM_DONGLE, HID_QUIRK_NOGET 
> },


-- 
Oliver Neukum 

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

Re: [PATCH 1/2] INPUT/HID: add touch support for SiS touch driver

[Oliver Neukum]

 .address_list   = normal_i2c,
> +#endif
> + .id_table   = sis_ts_id,
> + .driver = {
> + .name   = SIS_I2C_NAME,
> + },
> +};
> +
> +static int __init sis_ts_init(void)
> +{
> + pr_info("sis_ts_init\n");
> + sis_wq = create_singlethread_workqueue("sis_wq");
> +
> + if (!sis_wq)
> + return -ENOMEM;
> + return i2c_add_driver(&sis_ts_driver);
> +}
> +
> +static void __exit sis_ts_exit(void)
> +{
> + pr_info("sis_ts_exit\n");
> + i2c_del_driver(&sis_ts_driver);
> + if (sis_wq)
> + destroy_workqueue(sis_wq);
> +}
> +
> +module_init(sis_ts_init);
> +module_exit(sis_ts_exit);
> +MODULE_DESCRIPTION("SiS 9200 Family Touchscreen Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/linux-3.18.5/drivers/input/touchscreen/sis_i2c.h 
> b/linux-3.18.5/drivers/input/touchscreen/sis_i2c.h
> new file mode 100644
> index 000..7639a52
> --- /dev/null
> +++ b/linux-3.18.5/drivers/input/touchscreen/sis_i2c.h
> @@ -0,0 +1,173 @@
> +/*
> + * include/linux/sis_i2c.h - platform data structure for SiS 9200 family
> + *
> + * Copyright (C) 2011 SiS, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * Date: 2015/01/15
> + * Version:  Android_v2.05.00-A639-1113
> + */
> +#include 
> +
> +#ifndef _LINUX_SIS_I2C_H
> +#define _LINUX_SIS_I2C_H
> +
> +
> +#define SIS_I2C_NAME "sis_i2c_ts"
> +#define SIS_SLAVE_ADDR   0x5c
> +/*10ms*/
> +#define TIMER_NS 1000
> +#define MAX_FINGERS  10
> +
> +/* For standard R/W IO ( SiS firmware application )*/
> +#define _STD_RW_IO   /*ON/OFF*/
> +
> +/* Check data CRC */
> +/*#define _CHECK_CRC*/   
> /*ON/OFF*/
> +
> +/* Interrupt setting and modes */
> +#define _I2C_INT_ENABLE  /*ON/OFF*/
> +#define GPIO_IRQ 133
> +
> +/*   Enable if use interrupt case 1 mode.*/
> +/*   Disable if use interrupt case 2 mode.   */
> +/*#define _INT_MODE_1*/  /*ON/OFF*/
> +
> +/* Resolution mode */
> +/*Constant value*/
> +#define SIS_MAX_X4095
> +#define SIS_MAX_Y4095
> +
> +#define ONE_BYTE 1
> +#define FIVE_BYTE5
> +#define EIGHT_BYTE   8
> +#define SIXTEEN_BYTE 16
> +#define PACKET_BUFFER_SIZE   128
> +
> +#define SIS_CMD_NORMAL   0x0
> +#define SIS_CMD_SOFTRESET0x82
> +#define SIS_CMD_RECALIBRATE  0x87
> +#define SIS_CMD_POWERMODE0x90
> +#define MSK_TOUCHNUM 0x0f
> +#define MSK_HAS_CRC  0x10
> +#define MSK_DATAFMT  0xe0
> +#define MSK_PSTATE   0x0f
> +#define MSK_PID  0xf0
> +#define RES_FMT  0x00
> +#define FIX_FMT  0x40
> +
> +/* for

Re: [PATCH 1/2] INPUT/HID: add touch support for SiS touch driver

[Oliver Neukum]

On Fri, 2015-01-16 at 18:59 +0800, 曾婷葳 (tammy_tseng) wrote:
> Hey, Oliver
> 
> On Mon, Jan 12, 2015 at 7:50 PM, Oliver Neukum  wrote:
> On Mon, 2015-01-12 at 18:53 +0800, 曾婷葳 (tammy_tseng) wrote:
> > > (Skip the code diff...)
> > 
> > Again macros for endianness
> > 
> > And the driver has a great number of conditional compilations are they 
> > really needed? The driver as is has a number of issues and is hard to 
> > review due to the use of "//" for comments and a lot of conditional 
> > compilation and unnecessary variables used for constants. Could you 
> > fix this up and resubmit?
> 
> Thanks for the reply. I've modified the code 
> (sis_i2c.c/sis_i2c.h/Kconfig/Makefile) to fix them.
> Please help check them if anything needs to fix.
> Thanks.

You still have conditional compilation needlessly. You still haven't
addressed the issues I told you about during the review of the last
version.

Oliver

> 
> Tammy
> 
> ---
> diff --git a/linux-3.18.1/drivers/input/touchscreen/Kconfig 
> b/linux-3.18.1/drivers/input/touchscreen/Kconfig
> index e1d8003..401cd8b 100644
> --- a/linux-3.18.1/drivers/input/touchscreen/Kconfig
> +++ b/linux-3.18.1/drivers/input/touchscreen/Kconfig
> @@ -962,4 +962,12 @@ config TOUCHSCREEN_ZFORCE
> To compile this driver as a module, choose M here: the
> module will be called zforce_ts.
>  
> +config TOUCHSCREEN_SIS_I2C
> + tristate "SiS 9200 family I2C touchscreen driver"
> + depends on I2C
> + default n
> + help
> +   This enables support for SiS 9200 family over I2C based touchscreens.
> +
> +
>  endif
> diff --git a/linux-3.18.1/drivers/input/touchscreen/Makefile 
> b/linux-3.18.1/drivers/input/touchscreen/Makefile
> index 090e61c..67137af 100644
> --- a/linux-3.18.1/drivers/input/touchscreen/Makefile
> +++ b/linux-3.18.1/drivers/input/touchscreen/Makefile
> @@ -79,3 +79,4 @@ obj-$(CONFIG_TOUCHSCREEN_WM97XX_ZYLONITE)   += 
> zylonite-wm97xx.o
>  obj-$(CONFIG_TOUCHSCREEN_W90X900)+= w90p910_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_TPS6507X)   += tps6507x-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_ZFORCE) += zforce_ts.o
> +obj-$(CONFIG_TOUCHSCREEN_SIS_I2C)+= sis_i2c.o
> diff --git a/linux-3.18.1/drivers/input/touchscreen/sis_i2c.c 
> b/linux-3.18.1/drivers/input/touchscreen/sis_i2c.c
> new file mode 100644
> index 000..157f991
> --- /dev/null
> +++ b/linux-3.18.1/drivers/input/touchscreen/sis_i2c.c
> @@ -0,0 +1,1267 @@
> +/*
> + * SiS 9200 family I2C Touch Screen Controller Driver
> + *
> + * Copyright (C) 2011 SiS, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include 
> +#include 
> +#ifdef CONFIG_HAS_EARLYSUSPEND
> +#include 
> +#endif
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "sis_i2c.h"
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#ifdef _STD_RW_IO
> +#include 
> +#include 
> +#include 
> +#define DEVICE_NAME "sis_aegis_touch_device"
> +static int sis_char_devs_count = 1;/* device count */
> +static int sis_char_major = 0;
> +static struct cdev sis_char_cdev;
> +static struct class *sis_char_class = NULL;
> +#endif
> +
> +/* Addresses to scan */
> +static const unsigned short normal_i2c[] = { SIS_SLAVE_ADDR, I2C_CLIENT_END 
> };
> +static struct workqueue_struct *sis_wq;
> +struct sis_ts_data *ts_bak = 0;
> +struct sisTP_driver_data *TPInfo = NULL;
> +static void sis_tpinfo_clear(struct sisTP_driver_data *TPInfo, int max);
> +
> +#ifdef CONFIG_HAS_EARLYSUSPEND
> +static void sis_ts_early_suspend(struct early_suspend *h);
> +static void sis_ts_late_resume(struct early_suspend *h);
> +#endif
> +
> +#ifdef CONFIG_X86
> +/* static const struct i2c_client_address_data addr_data; */
> +/* Insmod parameters */
> +static int sis_ts_detect(struct i2c_client *client, struct i2c_board_info 
> *info);
> +#endif
> +
> +#ifdef _CHECK_CRC
> +uint16_t cal_crc (char* cmd, int start, int end);
> +#endif
> +
> +static void PrintBuffer(int start, int length, char* buf)
> +{
> + int i;
> + for ( i = start; i < 

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Oliver Neukum]

On Mon, 2015-02-09 at 20:44 +0200, Lauri Kasanen wrote:
> On Mon, 09 Feb 2015 11:08:01 +0100
> Oliver Neukum  wrote:
> 
> > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02),
> > > + buf2, sizeof(buf2),
> > > + &transfered, USB_CTRL_SET_TIMEOUT);
> > 
> > You cannot do this. Even for a single byte DMA on the stack is
> > wrong. Not on all architectures it works at all and you violate
> > the DMA constrainsts. You must use kmalloc().
> 
> Hi Oliver,
> 
> Does this still apply when using hid_hw_output_report?

Yes. For USB devices hid_hw_output_report() goes to
usbhid_output_report(). That goes to usb_interrupt_msg(),
which passes the buffer pointer. It will then be mapped
for DMA. You must not do that on the stack.

HTH
Oliver


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

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Oliver Neukum]

On Sat, 2015-02-07 at 15:48 +0200, Lauri Kasanen wrote:
> Without this, my "Gasia Co.,Ltd PS(R) Gamepad" would not send
> any events. Now everything works including the leds.
> 
> Based on work by Andrew Haines and Antonio Ospite.
> 
> cc: Antonio Ospite 
> cc: Andrew Haines 
> Signed-off-by: Lauri Kasanen 
> ---
>  drivers/hid/hid-sony.c | 26 --
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> Antonio's original approach was not enough; it enabled the events,
> but only for a few seconds, then the controller timed out and sent
> no more. Andrew's did more than was necessary. This is a combination
> of the two, against Linus' git.
> 
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 31e9d25..de93386 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -36,6 +36,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "hid-ids.h"
>  
> @@ -1130,8 +1131,12 @@ static void sony_input_configured(struct hid_device 
> *hdev,
>   */
>  static int sixaxis_set_operational_usb(struct hid_device *hdev)
>  {
> + struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> + struct usb_device *dev = interface_to_usbdev(intf);
>   int ret;
> - char *buf = kmalloc(18, GFP_KERNEL);
> + char *buf = kmalloc(65, GFP_KERNEL);
> + unsigned char buf2[] = { 0x00 };
> + int transfered;
>  
>   if (!buf)
>   return -ENOMEM;
> @@ -1140,7 +1145,24 @@ static int sixaxis_set_operational_usb(struct 
> hid_device *hdev)
>HID_REQ_GET_REPORT);
>  
>   if (ret < 0)
> - hid_err(hdev, "can't set operational mode\n");
> + hid_err(hdev, "can't set operational mode on the control EP\n");
> +
> + /*
> +  * Some compatible controllers like the Speedlink Strike FX and
> +  * Gasia need another query plus an USB interrupt to get operational.
> +  */
> + ret = hid_hw_raw_request(hdev, 0xf5, buf, 64, HID_FEATURE_REPORT,
> +  HID_REQ_GET_REPORT);
> +
> + if (ret < 0)
> + hid_err(hdev, "can't set operational mode on the interrupt 
> EP\n");
> +
> + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02),
> + buf2, sizeof(buf2),
> + &transfered, USB_CTRL_SET_TIMEOUT);

You cannot do this. Even for a single byte DMA on the stack is
wrong. Not on all architectures it works at all and you violate
the DMA constrainsts. You must use kmalloc().

Regards
Oliver


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

Re: How to fix CDROM/DVD eject mess?

[Oliver Neukum]

On Mon, 2015-02-02 at 20:45 +0100, Kay Sievers wrote:
> >  All the technical details aside, this is a bold statement -- how do
> you
> > know what the user actually wants?
> 
> By working with people who spent a lot of time with the questions what
> the default behavior of user interfaces should be. Buttons, especially
> physical ones, need to give immediate feedback to the user. If they
> don't give it it, people will look for something else to get what they
> want.

Nevertheless this is a policy decision and doesn't belong into udev.

Regards
Oliver


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

Re: USB autosuspend causing trouble with bluetooth

[Oliver Neukum]

On Mon, 2015-01-26 at 21:00 +0400, Kirill Elagin wrote:
> Things just got way worse with this hotplug thing.

Is that a new test or did you update?

> When the host power/control is set to `auto`, as soon as I insert  the
> Unifying receiver one of kworkers starts eating 100% of one of the

But it does not enumerate the dongle, does it?

> cores, according to `htop`. As soon as I `echo on` to power/control of
> the relevant USB hub the kworker stops doing this. This also happens
> with that other USB1.1 BT-dongle, so I assume any USB1.1 device would
> do. Also just unplugging the device is not enough, the kworker keeps
> eating CPU until I `echo on` to power/control.
> 
> Should I start a separate thread?
> Right now I'm on 3.18.2-gentoo.

Can you tell at which kernel version this started?

Regards
Oliver


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

Re: USB autosuspend causing trouble with bluetooth

[Oliver Neukum]

On Tue, 2015-01-20 at 23:25 +0400, Kirill Elagin wrote:

> Hm, I'm pretty sure I never touched anything with `port` in its name,
> all the ports are set to `auto` (that's what laptop-mode-tools does).

Here we go.

> Right now I think I see three possibly unrelated issues:
> 
> Issue #1. BT trackpad not working properly when connected to the
> builtin bluetooth adapter.
> --
> 
> The adapter is attached to a USB1.1 hub:
> 
> 
> # lsusb -t
> ...
> /:  Bus 03.Port 1: Dev 1, Class=root_hub, Driver=uhci_hcd/2p, 12M
> |__ Port 2: Dev 2, If 0, Class=Wireless, Driver=btusb, 12M
> |__ Port 2: Dev 2, If 1, Class=Wireless, Driver=btusb, 12M
> |__ Port 2: Dev 2, If 2, Class=Vendor Specific Class, Driver=, 12M
> |__ Port 2: Dev 2, If 3, Class=Application Specific Interface, Driver=, 
> 12M
> ...
> 
> # cat usb3/power/control
> auto
> 
> # cat usb3/3-*/usb3-port*/power/control
> auto
> auto

Try setting this to on.

> # cat usb3/3-2/power/control
> on

Here "auto" should work. Please try.
[..]
> Issue #2. No hotplug with USB1.1:
> --
> 
> To see this I pick one physical port. When I plug a USB1.1 device it
> appears on bus 4 port 2; a USB2.0 device appears on bus 1 port 4.
> 
> 
> # cat usb4/power/control
> auto
> # cat usb4/4-*/usb4-port*/power/control
> auto
> auto

Please set this to "on"

> # cat usb1/power/control
> auto
> # cat usb1/1-*/usb1-port*/power/control

Please set this to "on"

[..]
> 
> Issue #3. No hot-plug-out for USB1.1.
> 

> I think that the first two issues are fixed by keeping all the USB1.1
> hubs and the builtin BT always `on`, but I just wanted to know whether
> those are hardware or software bugs.

I suspect this is caused by outdated laptop-mode-tools.

Basically setting the port controls (as opposed to hub and device
controls) to "auto" tells the kernel that it may disable hotplugging
to save energy. Hotunplug for devices that need remote wakeup will
still work. Likewise if you disable autosuspend of the attached devices.

Regards
Oliver


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

Re: USB autosuspend causing trouble with bluetooth

[Oliver Neukum]

On Tue, 2015-01-20 at 18:58 +0400, Kirill Elagin wrote:
> On Tue, Jan 20, 2015 at 5:06 PM, Oliver Neukum  wrote:
> > On Tue, 2015-01-20 at 16:18 +0400, Kirill Elagin wrote:
> >> I use a Logitech wireless keyboard (with a Unifying receiver) and it
> >> keeps working fine even with `auto`.
> >>
> >> That is, everything is OK if the receiver is plugged before
> >> `power/control` is switched to `auto`.
> >
> > Wait. There is no power/control file for the receiver before
> > you plug it in. We are having a very big misunderstanding here.
> 
> Sorry for not being clear. I was referring to `power/control` of the
> USB-device itself except for the cases when I was talking about
> hot-plugging issues — in those cases I was referring to the
> `power/control` of the root hub.

Please check whether you are not accidentally touching the ports
linux-0dmf:/sys/bus/usb/devices/usb1/1-0:1.0/usb1-port1
At paths like this you find control files for ports, not the
root hub as a device.

> In this particular case I was talking about the `power/control` of the root 
> hub.

OK, so autosuspend does work if you enable it for the device but
not the hub?

> `laptop-mode-tools` by default writes `auto` to `power/control` of
> _all_ the USB devices, root hubs included (even when on AC). Is it
> really expected that kernel might completely power off the physical
> USB port? Sounds weird.

It can. It is a recent feature if ACPI supports that on a machine.

> Here is an even more strange thing. First I set all the USB power
> management to the defaults (that is, `auto` for all the usb devices
> including root hubs). Again, the keyboard keeps working and as soon as
> I unplug the receiver kernel says the device was disconnected. Now if
> I plug the receiver back nothing happens. _But_ if I plug a flash
> drive in the save physical port it gets detected. So, I tried a number
> of other usb devices and it totally looks like USB2.0 ones are
> properly hot-plugged while USB1.1 devices are not. Does this sound to
> you like a bug in my laptop's hardware?

Could be, but could also be software.
Please double check where exactly an "on" is needed.

Regards
Oliver


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

Re: USB autosuspend causing trouble with bluetooth

[Oliver Neukum]

On Tue, 2015-01-20 at 16:18 +0400, Kirill Elagin wrote:
> I use a Logitech wireless keyboard (with a Unifying receiver) and it
> keeps working fine even with `auto`.
> 
> That is, everything is OK if the receiver is plugged before
> `power/control` is switched to `auto`.

Wait. There is no power/control file for the receiver before
you plug it in. We are having a very big misunderstanding here.

> But if I first set it to `auto`, then plug the receiver in, it is not
> detected (nothing in dmesg). Kernel
> detects it as soon as I `echo on` to the relevant `power/control`.

Are you on power/control for the device or the port?
If you are using the port's control file you might be switching
on Port Power Off. Then of course the port will not process
hotplugs. Please clarify!
> 
> This laptop is too old to have USB3.0, both the receiver and BT are
> attached to USB1.1 ports.
> BTW I also noticed a strange thing: USB devices appear on different
> buses when attached,
> depending on their speed (e.g. the keyboard receiver is on bus 6 which
> is USB1.1, while a
> USB stick appears on bus 2 which is USB2.0 when I plug it into that
> same physical port).
> I’m not sure whether this is strange or normal, as I never really
> payed attention.

That is perfectly normal.

Regards
Oliver

PS: Please dont top-post


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

Re: [PATCH 11/11] usb: core: fix a race with usb_queue_reset_device()

[Oliver Neukum]

On Tue, 2015-01-20 at 13:29 +0100, Olivier Sobrie wrote:
> When usb_queue_reset() is called it schedules a work in view of
> resetting the usb interface. When the reset work is running, it
> can be scheduled again (e.g. by the usb disconnect method of
> the driver).
> 
> Consider that the reset work is queued again while the reset work
> is running and that this work leads to a forced unbinding of the
> usb interface (e.g. because a driver is bound to the interface
> and has no pre/post_reset methods - see usb_reset_device()).
> In such condition, usb_unbind_interface() gets called and this
> function calls usb_cancel_queued_reset() which does nothing
> because the flag "reset_running" is set to 1. The second reset
> work that has been scheduled is therefore not cancelled.
> Later, the usb_reset_device() tries to rebind the interface.
> If it fails, then the usb interface context which contain the
> reset work struct is freed and it most likely crash when the
> second reset work tries to be run.
> 
> The following flow shows the problem:
> * usb_queue_reset_device()
> * __usb_queue_reset_device() <- If the reset work is queued after here, then
> reset_running = 1   it will never be cancelled.
> usb_reset_device()
>   usb_forced_unbind_intf()
> usb_driver_release_interface()
>   usb_unbind_interface()
> driver->disconnect()
>   usb_queue_reset_device() <- second reset

That is the sledgehammer approach. Wouldn't it be better to guarantee
that usb_queue_reset_device() be a nop when reset_running==1 ?

> usb_cancel_queued_reset() <- does nothing because
>  the flag reset_running
>  is set
>   usb_unbind_and_rebind_marked_interfaces()
> usb_rebind_intf()
>   device_attach()
> driver->probe() <- fails (no more drivers hold a reference to
> the usb interface)
> reset_running = 0
> * hub_event()
> usb_disconnect()
>   usb_disable_device()
> kobject_release()
>   device_release()
> usb_release_interface()
>   kfree(intf) <- usb interface context is released
>  while we still have a pending reset
>  work that should be run
> 
> To avoid this problem, we use a delayed work so that if the reset
> work is currently run, we can avoid further call to
> __usb_queue_reset_device() work by using cancel_delayed_work().
> Unfortunately it increases the size of the usb_interface structure...

Regards
Oliver

-- 
Oliver Neukum 

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

Re: [PATCH 04/11] hso: fix memory leak in hso_create_rfkill()

[Oliver Neukum]

On Tue, 2015-01-20 at 13:29 +0100, Olivier Sobrie wrote:
> When the rfkill interface was created, a buffer containing the name
> of the rfkill node was allocated. This buffer was never freed when the
> device disappears.
> 
> To fix the problem, we put the name given to rfkill_alloc() in
> the hso_net structure.
> 
> Signed-off-by: Olivier Sobrie 
> ---
>  drivers/net/usb/hso.c | 12 +++-
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> index 470ef9e..a49ac2e 100644
> --- a/drivers/net/usb/hso.c
> +++ b/drivers/net/usb/hso.c
> @@ -153,6 +153,7 @@ struct hso_net {
>   struct hso_device *parent;
>   struct net_device *net;
>   struct rfkill *rfkill;
> + char name[8];
>  
>   struct usb_endpoint_descriptor *in_endp;
>   struct usb_endpoint_descriptor *out_endp;
> @@ -2467,27 +2468,20 @@ static void hso_create_rfkill(struct hso_device 
> *hso_dev,
>  {
>   struct hso_net *hso_net = dev2net(hso_dev);
>   struct device *dev = &hso_net->net->dev;
> - char *rfkn;
>  
> - rfkn = kzalloc(20, GFP_KERNEL);
> - if (!rfkn)
> - dev_err(dev, "%s - Out of memory\n", __func__);
> -
> - snprintf(rfkn, 20, "hso-%d",
> + snprintf(hso_net->name, sizeof(hso_net->name), "hso-%d",
>interface->altsetting->desc.bInterfaceNumber);

That number is not unique. Indeed it will be identical for all devices.

Regards
Oliver

-- 
Oliver Neukum 

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

Re: [PATCH v2 0/4] toshiba_acpi: Add support for USB Sleep functions

[Oliver Neukum]

On Sun, 2015-01-18 at 18:30 -0700, Azael Avalos wrote:
> The following patches add support to several USB Sleep functions
> found on newer Toshiba laptops, allowing to use the USB ports while
> the laptop is asleep or turned off.

Hi,

this is most interesting. But the interface is terrible. If
possible we would like to have a generic interface for this,
which should not depend on the specific platform in use and would
have to be per bus or even per port (for example the ports on a
Thunderbolt dock would not work with your module).
Do you think we could provide a hook from generic code into
platform code to detect the capability for power control?

Regards
Oliver



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

Re: USB autosuspend causing trouble with bluetooth

[Oliver Neukum]

On Sun, 2015-01-18 at 17:30 +0400, Kirill Elagin wrote:
> Hello,
> 
> Recently I started having issues with my Apple Magic Trackpad and I
> realised that the problem was with autosuspend. Whenever I have `auto`
> in `power/control` of my BT adapter, `btmon` shows no packets,
> nothing. As soon as I `echo on`, all the missing packets arrive.

You are not getting remote wakeups. There are two possibilities

1. the firmware of your BT adapter is faulty and the device needs to
be added to the list of quirky devices

2. a bug in the kernel breaks remote wakeup.

We need to distinguish these cases. Could you connect another device
that uses remote wakeup (HID, CDC-ACM, ... - a keyboard or a mouse is
easiest) to a port connected to XHCI and test autosuspend on that
device?

Regards
Oliver




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

Re: [PATCH 1/2] INPUT/HID: add touch support for SiS touch driver

[Oliver Neukum]

On Fri, 2015-01-16 at 18:59 +0800, 曾婷葳 (tammy_tseng) wrote:
> Hey, Oliver
> 
> On Mon, Jan 12, 2015 at 7:50 PM, Oliver Neukum  wrote:
> On Mon, 2015-01-12 at 18:53 +0800, 曾婷葳 (tammy_tseng) wrote:
> > > (Skip the code diff...)
> > 
> > Again macros for endianness
> > 
> > And the driver has a great number of conditional compilations are they 
> > really needed? The driver as is has a number of issues and is hard to 
> > review due to the use of "//" for comments and a lot of conditional 
> > compilation and unnecessary variables used for constants. Could you 
> > fix this up and resubmit?
> 
> Thanks for the reply. I've modified the code 
> (sis_i2c.c/sis_i2c.h/Kconfig/Makefile) to fix them.
> Please help check them if anything needs to fix.

Hi,

I started from the lower end. I hope you find my comments
useful. A few general points need to be made before the
code is ready for review (it is obviously not ready for
inclusion)

1. We don't do conditional compilation unless we absolutely have
to in the kernel. The support for earlier kernel versions has
to be ripped out of the code.

2. You must use the symbolic names of error codes. They are not
guaranteed to be uniform among architectures.

3. If you need to sleep at places without obvious need, you need
to include comments explaining the reason.

4. No comments with "//"

Please redo these things and resubmit.

Regards
Oliver

> +/* for get system time */
> +static ssize_t sis_cdev_read( struct file *file, char __user *buf, size_t 
> count, loff_t *f_pos )
> +{
> +  int ret = 0;
> +  char *kdata;
> +  char cmd;
> +  int i;
> +  
> +  printk(KERN_INFO "sis_cdev_read.\n");
> +  
> +  if (ts_bak == 0)
> + return -13;
symbolic name necessary

> + 
> +ret = access_ok(VERIFY_WRITE, buf, BUFFER_SIZE);
> +if (!ret) {
> +printk(KERN_ERR "cannot access user space memory\n");
> +return -11;
symbolic name necessary
> +}
> +
> +  kdata = kmalloc(BUFFER_SIZE, GFP_KERNEL);
> + if (kdata == 0)
> + return -12;
symbolic name necessary
> + 
> + ret = copy_from_user(kdata, buf, BUFFER_SIZE);
> + if (ret) {
> +printk(KERN_ERR "copy_from_user fail\n");
> +kfree(kdata);
> +return -14;
symbolic name necessary
> + }
> +#if 0
> +PrintBuffer(0, count, kdata);
> +#endif
> +  cmd = kdata[6];
> +  /* for making sure AP communicates with SiS driver */
> +if(cmd == 0xa2)
> +{
> + kdata[0] = 5;
> + kdata[1] = 0;
> + kdata[3] = 'S';
> + kdata[4] = 'i';
> + kdata[5] = 'S';
> + if ( copy_to_user((char*) buf, kdata, BUFFER_SIZE ) )
What is happening here?
> + {
> + printk(KERN_ERR "copy_to_user fail\n" );
> + kfree( kdata );
> + return -19;
symbolic name needed
> + }
> +
> + kfree( kdata );
> + return 3;
Why?
>   
> + }
> +/* Write & Read */
> +ret = sis_command_for_read(ts_bak->client, MAX_BYTE, kdata);
> +if (ret < 0) {
> +printk(KERN_ERR "i2c_transfer read error %d\n", ret);
> + kfree(kdata);
> + return -21;
symbolic name needed
> + }
> +
> + ret = kdata[0] | (kdata[1] << 8);
Use the macros endianness conversion.

In this case
ret = le16_to_cpu(kdata + 0);

> +printk(KERN_INFO "%d\n", ret);
> +
> +for ( i = 0; i < ret && i < BUFFER_SIZE; i++ )
> +{
> +printk("%02x ", kdata[i]);
> +}
> +
> +printk( "\n" );
> +
> +if ( copy_to_user((char*) buf, kdata, BUFFER_SIZE ) )
> +{
> +printk(KERN_ERR "copy_to_user fail\n" );
> +ret = -19;
What is this to mean? I suppose it is an error code. The symbolic
values must be used.
> +}
> +
> +kfree( kdata );
> +
> + return ret;
> +}
> +
> +#undef BUFFER_SIZE
> +
> +static int sis_cdev_open(struct inode *inode, struct file *filp)
> +{
> + printk(KERN_INFO "sis_cdev_open.\n");
> + if ( ts_bak == 0 )
> + return -13;

What is this to mean?
> +
> + msleep(200);

Why?
> + if (ts_bak->use_irq)
> + {
> +#if ( LINUX_VERSION_CODE < KERNEL_VERSION (2, 6, 39) )
> +if ((ts_bak->desc->status & IRQ_DISABLED) == IRQ_STATUS_ENABLED)
> +#else
> + if ((ts_bak->desc->irq_data.state_use_a

Re: [PATCH] usb: Fix typo in `struct usb_host_interface' comment

[Oliver Neukum]

On Thu, 2015-01-15 at 17:47 -0600, Chris Rorvick wrote:
> I think understand your comment now so I will respond more
> specifically:
> This is a typo precisely because `bNumEndpoint' is not consistent with
> the specification, nor is it consistent with the definition of `struct
> usb_interface_descriptor'.  The specification and the structure both
> use
> `bNumEndpoints'.

Sorry, I stand corrected. The patch is good.

Regards
Oliver


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

Re: [PATCH] usb: Fix typo in `struct usb_host_interface' comment

[Oliver Neukum]

On Wed, 2015-01-14 at 21:52 -0600, Chris Rorvick wrote:
> The descriptor member `bNumEndpoints' is plural.

I am afraid that is not a good idea. The name of a
member of a structure mentioned in the specification should
appear as it is used in the specification.

Regards
Oliver


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

Re: [PATCH] usb/kaweth: use GFP_ATOMIC under spin_lock in usb_start_wait_urb()

[Oliver Neukum]

On Sat, 2015-01-10 at 02:16 +0300, Alexey Khoroshilov wrote:
> Commit e4c7f259c5be ("USB: kaweth.c: use GFP_ATOMIC under spin_lock")
> makes sure that kaweth_internal_control_msg() allocates memory with
> GFP_ATOMIC,
> but kaweth_internal_control_msg() also calls usb_start_wait_urb()
> that still allocates memory with GFP_NOIO.
> 
> The patch fixes usb_start_wait_urb() as well.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Alexey Khoroshilov 

Acked-by: Oliver Neukum 

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

Re: [PATCH 1/2] INPUT/HID: add touch support for SiS touch driver

[Oliver Neukum]

On Mon, 2015-01-12 at 18:53 +0800, 曾婷葳 (tammy_tseng) wrote:
> Hi,
> This package of patch is to add support for multitouch behavior for SiS touch 
> products.
> The patch of SiS i2c multitouch driver is in input/touchscreen.
> 
> Signed-off-by: Tammy Tseng 
> 
> ---
> diff --git a/linux-3.18.1/drivers/input/touchscreen/Kconfig 
> b/linux-3.18.1/drivers/input/touchscreen/Kconfig
> index e1d8003..edc8e27 100644
> --- a/linux-3.18.1/drivers/input/touchscreen/Kconfig
> +++ b/linux-3.18.1/drivers/input/touchscreen/Kconfig
> @@ -962,4 +962,18 @@ config TOUCHSCREEN_ZFORCE
> To compile this driver as a module, choose M here: the
> module will be called zforce_ts.
>  
> +config TOUCHSCREEN_SIS_I2C
> + tristate "SiS 9200 family I2C touchscreen driver"
> + depends on I2C
> + default y

Why that default? The majority of systems will not feature this
touchscreens.

> + help
> +   This enables support for SiS 9200 family over I2C based touchscreens.
> +
> +config FW_SUPPORT_POWERMODE
> + default n
> +bool "SiS FW support power mode"
> +depends on TOUCHSCREEN_SIS_I2C
> +help
> +  This enables support power mode provided by SiS firmwave

What does this mode do? The help should be more extensive to be
helpful.

> +
>  endif
> diff --git a/linux-3.18.1/drivers/input/touchscreen/Makefile 
> b/linux-3.18.1/drivers/input/touchscreen/Makefile
> index 090e61c..e316477 100644
> --- a/linux-3.18.1/drivers/input/touchscreen/Makefile
> +++ b/linux-3.18.1/drivers/input/touchscreen/Makefile
> @@ -6,6 +6,10 @@
>  
>  wm97xx-ts-y := wm97xx-core.o
>  
> +ifdef CONFIG_TOUCHSCREEN_SIS_I2C
> +obj-$(CONFIG_TOUCHSCREEN_SIS_I2C)   += sis_i2c.o
> +endif
> +
>  obj-$(CONFIG_OF_TOUCHSCREEN) += of_touchscreen.o
>  obj-$(CONFIG_TOUCHSCREEN_88PM860X)   += 88pm860x-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_AD7877) += ad7877.o
> diff --git a/linux-3.18.1/drivers/input/touchscreen/sis_i2c.c 
> b/linux-3.18.1/drivers/input/touchscreen/sis_i2c.c
> new file mode 100644
> index 000..2e6fc1a
> --- /dev/null
> +++ b/linux-3.18.1/drivers/input/touchscreen/sis_i2c.c
> @@ -0,0 +1,1725 @@
> +/* drivers/input/touchscreen/sis_i2c.c - I2C Touch panel driver for SiS 9200 
> family
> + *
> + * Copyright (C) 2011 SiS, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * Date: 2012/11/13
> + * Version:  Android_v2.05.00-A639-1113
> + */
> +
> +#include 
> +#include 
> +#ifdef CONFIG_HAS_EARLYSUSPEND
> +#include 
> +#endif

Conditional includes should be avoided if possible.

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "sis_i2c.h"
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#ifdef _STD_RW_IO

???

> +#include 
> +#include 
> +#include 
> +#define DEVICE_NAME "sis_aegis_touch_device"
> +static int sis_char_devs_count = 1;/* device count */

Why start with 1 ?
> +static int sis_char_major = 0;
> +static struct cdev sis_char_cdev;
> +static struct class *sis_char_class = NULL;
> +#endif
> +
> +/* Addresses to scan */
> +static const unsigned short normal_i2c[] = { SIS_SLAVE_ADDR, I2C_CLIENT_END 
> };
> +static struct workqueue_struct *sis_wq;
> +struct sis_ts_data *ts_bak = 0;
> +struct sisTP_driver_data *TPInfo = NULL;

Are you really sure you are limited to a single device?
> +static void sis_tpinfo_clear(struct sisTP_driver_data *TPInfo, int max);
> +
> +#ifdef CONFIG_HAS_EARLYSUSPEND
> +static void sis_ts_early_suspend(struct early_suspend *h);
> +static void sis_ts_late_resume(struct early_suspend *h);
> +#endif
> +
> +#ifdef CONFIG_X86
> +//static const struct i2c_client_address_data addr_data;
> +/* Insmod parameters */
> +static int sis_ts_detect(struct i2c_client *client, struct i2c_board_info 
> *info);
> +#endif
> +
> +#ifdef _CHECK_CRC
> +uint16_t cal_crc (char* cmd, int start, int end);
> +#endif
> +
> +void PrintBuffer(int start, int length, char* buf)

Polluting the name space like this is really nasty.
Could you check which of your declarations can be
declared "static"?

> +{
> + int i;
> + for ( i = start; i < length; i++ )
> + {
> + printk("%02x ", buf[i]);
> + if (i != 0 && i % 30 == 0)
> + printk("\n");
> + }
> + printk("\n");   
> +}
> +
> +int sis_command_for_write(struct i2c_client *client, int wlength, unsigned 
> char *wdata)
> +{
> +int ret = -1;
> +struct i2c_msg msg[1];

Why on earth an array of a single element?

> +
> +msg[0].addr = client->addr;
> +msg[0].fl

Re: [PATCH 0/2] INPUT/HID: add touch support for SiS touch driver

[Oliver Neukum]

On Fri, 2015-01-09 at 18:36 +0800, 曾婷葳 (tammy_tseng) wrote:
> Signed-off-by: Tammy Tseng 
> 
> ---
> The patch for i2c multitouch driver in input/touchscreen:
> 
> linux-3.18.1/drivers/input/touchscreen/Kconfig   |   14 -
>  linux-3.18.1/drivers/input/touchscreen/Makefile  |4 -
>  linux-3.18.1/drivers/input/touchscreen/sis_i2c.c | 1725
> --
>  linux-3.18.1/drivers/input/touchscreen/sis_i2c.h |  229 ---
>  4 files changed, 1972 deletions(-)

Only deletions and it adds support?
I guess you mixed up your trees.

Regards
Oliver



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

Re: [PATCH v2 0/4] input: synaptics - report correct width and pressure values

[Oliver Neukum]

On Mon, 2015-01-05 at 23:28 +0100, Gabriele Mazzotta wrote:
> Make image sensors and cr48 sensors report widths and fix the
> calculation of width and pressure values in some cases.
> 
> Changes since v1:
>  - Rebased on 35393dcb2ed3
>  - Get widths from AGM packets too
>  - Include support for cr48 sensors
>  - Fix pressure values on image sensors

Hi,

it seems to me that all these fixes should go into stable.

Regards
Oliver



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

Re: [PATCH 4/4 v4] scsi:stex.c Add S3/S4 support

[Oliver Neukum]

On Tue, 2014-12-16 at 14:14 +0800, Charles Chiou wrote:
>  From f9d84df080c16097218092630db9b5df31d487b5 Mon Sep 17 00:00:00 2001
> From: Charles Chiou 
> Date: Fri, 7 Nov 2014 10:15:18 +0800
> Subject: [PATCH 4/4] scsi:stex.c Add S3/S4 support
> 
> Add S3/S4 support, add .suspend and .resume function in pci_driver.
> 
> Pegasus need 30~40 seconds to boot up. We don't want to OS wait
> in .resume function. Create a thread to handle device boot up.

I am sorry to be obnoxious, but this patch raises another
question. What happens if the the system is suspended again
while the work scheduled in resume is still running?

Furthermore, what happens in the case of a PCI hotunplug while
the work is still scheduled?

Regards
Oliver


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

Re: bluetooth: Add hci_h4p driver

[Oliver Neukum]

Hi,

a few remarks about possible issues.

Regards
Oliver

> +static int h4p_send_negotiation(struct h4p_info *info)
> +{
> + struct h4p_neg_cmd *neg_cmd;
> + struct h4p_neg_hdr *neg_hdr;
> + struct sk_buff *skb;
> + int err, len;
> + u16 sysclk = 38400;
> +
> + printk("Sending negotiation..");
> + len = sizeof(*neg_cmd) + sizeof(*neg_hdr) + H4_TYPE_SIZE;
> +
> + skb = bt_skb_alloc(len, GFP_KERNEL);
> + if (!skb)
> + return -ENOMEM;
> +
> + memset(skb->data, 0x00, len);
> + *skb_put(skb, 1) = H4_NEG_PKT;
> + neg_hdr = (struct h4p_neg_hdr *)skb_put(skb, sizeof(*neg_hdr));
> + neg_cmd = (struct h4p_neg_cmd *)skb_put(skb, sizeof(*neg_cmd));
> +
> + neg_hdr->dlen = sizeof(*neg_cmd);
> + neg_cmd->ack = H4P_NEG_REQ;
> + neg_cmd->baud = cpu_to_le16(BT_BAUDRATE_DIVIDER/MAX_BAUD_RATE);
> + neg_cmd->proto = H4P_PROTO_BYTE;
> + neg_cmd->sys_clk = cpu_to_le16(sysclk);
> +
> + h4p_change_speed(info, INIT_SPEED);
> +
> + h4p_set_rts(info, 1);
> + info->init_error = 0;
> + init_completion(&info->init_completion);
> +
> + h4p_simple_send_frame(info, skb);
> +
> + if (!wait_for_completion_interruptible_timeout(&info->init_completion,
> +msecs_to_jiffies(1000))) 
> {
> + printk("h4p: negotiation did not return\n");

Memory leak in the error case

> + return -ETIMEDOUT;
> + }
> +
> + if (info->init_error < 0)
> + return info->init_error;
> +
> + /* Change to operational settings */
> + h4p_set_auto_ctsrts(info, 0, UART_EFR_RTS);
> + h4p_set_rts(info, 0);
> + h4p_change_speed(info, MAX_BAUD_RATE);
> +
> + err = h4p_wait_for_cts(info, 1, 100);
> + if (err < 0)
> + return err;
> +
> + h4p_set_auto_ctsrts(info, 1, UART_EFR_RTS);
> + init_completion(&info->init_completion);
> + err = h4p_send_alive_packet(info);
> +
> + if (err < 0)
> + return err;
> +
> + if (!wait_for_completion_interruptible_timeout(&info->init_completion,
> + msecs_to_jiffies(1000)))
> + return -ETIMEDOUT;
> +
> + if (info->init_error < 0)
> + return info->init_error;
> +
> + printk("Negotiation successful\n");
> + return 0;
> +}



> +static unsigned int h4p_get_data_len(struct h4p_info *info,
> +  struct sk_buff *skb)
> +{
> + long retval = -1;
> + struct hci_acl_hdr *acl_hdr;
> + struct hci_sco_hdr *sco_hdr;
> + struct hci_event_hdr *evt_hdr;
> + struct h4p_neg_hdr *neg_hdr;
> + struct h4p_alive_hdr *alive_hdr;
> + struct h4p_radio_hdr *radio_hdr;
> +
> + switch (bt_cb(skb)->pkt_type) {
> + case H4_EVT_PKT:
> + evt_hdr = (struct hci_event_hdr *)skb->data;
> + retval = evt_hdr->plen;
> + break;
> + case H4_ACL_PKT:
> + acl_hdr = (struct hci_acl_hdr *)skb->data;
> + retval = le16_to_cpu(acl_hdr->dlen);

Could you explain, why only this needs endianness converted?

> + break;
> + case H4_SCO_PKT:
> + sco_hdr = (struct hci_sco_hdr *)skb->data;
> + retval = sco_hdr->dlen;
> + break;
> + case H4_RADIO_PKT:
> + radio_hdr = (struct h4p_radio_hdr *)skb->data;
> + retval = radio_hdr->dlen;
> + break;
> + case H4_NEG_PKT:
> + neg_hdr = (struct h4p_neg_hdr *)skb->data;
> + retval = neg_hdr->dlen;
> + break;
> + case H4_ALIVE_PKT:
> + alive_hdr = (struct h4p_alive_hdr *)skb->data;
> + retval = alive_hdr->dlen;
> + break;
> + }
> +
> + return retval;
> +}



> +static void h4p_rx_tasklet(unsigned long data)
> +{
> + u8 byte;
> + struct h4p_info *info = (struct h4p_info *)data;
> +
> + BT_DBG("tasklet woke up");
> + BT_DBG("rx_tasklet woke up");

Isn't this a bit redundant?

> +
> + while (h4p_inb(info, UART_LSR) & UART_LSR_DR) {
> + byte = h4p_inb(info, UART_RX);
> + BT_DBG("[in: %02x]", byte);
> + if (info->garbage_bytes) {
> + info->garbage_bytes--;
> + continue;
> + }
> + if (info->rx_skb == NULL) {
> + info->rx_skb = bt_skb_alloc(HCI_MAX_FRAME_SIZE,
> + GFP_ATOMIC | GFP_DMA);
> + if (!info->rx_skb) {
> + dev_err(info->dev,
> + "No memory for new packet\n");
> + goto finish_rx;
> + }
> + info->rx_state = WAIT_FOR_PKT_TYPE;
> + info->rx_skb->dev = (void *)info->hdev;
> + }
> + info->hdev->stat.byte_rx++;
> + h4p_hand

Re: [V3 PATCH 4/4] scsi:stex.c Add S3/S4 support

[Oliver Neukum]

On Mon, 2014-12-15 at 11:12 +0800, Charles Chiou wrote:
> 
> On 12/10/2014 05:02 PM, Oliver Neukum wrote:
> > On Wed, 2014-12-10 at 09:38 +0800, Charles Chiou wrote:
> >>   From 91868d4afe10533b8a4496075109e411100217bb Mon Sep 17 00:00:00 2001
> >> From: Charles Chiou 
> >> Date: Fri, 7 Nov 2014 10:15:18 +0800
> >> Subject: [PATCH 4/4] scsi:stex.c Add S3/S4 support
> >>
> >> Add S3/S4 support, add .suspend and .resume function in pci_driver.
> >>
> >> Pegasus need 30~40 seconds to boot up. We don't want to OS wait
> >> in .resume function. Create a thread to handle device boot up.
> >>
> >
> >> +static int stex_resume(struct pci_dev *pdev)
> >> +{
> >> +  struct st_hba *hba = pci_get_drvdata(pdev);
> >> +  struct hba_handshake_workstruct *hswork;
> >> +  int sts;
> >> +
> >> +  hba->mu_status = MU_STATE_STARTING;
> >> +  hswork = kzalloc(sizeof(struct hba_handshake_workstruct), GFP_KERNEL);
> >
> > The system is coming back from sleep. You cannot swap or page out
> > as disks may still be asleep. GFP_KERNEL is automatically changed
> > to GFP_NOIO. It would be nice to outright use GFP_NOIO.
> >
> >> +  INIT_WORK(&hswork->handshake_work, resume_handshake);
> >
> > Memory allocations can fail.
> > I suggest you allocate the memory in suspend(). There you can just
> > return -ENOMEM in the error case.
> >
> >
> Hi Oliver, sorry for the late reply.
> 
> Good point, could we move kzalloc function from suspend to probe and 
> return -ENOMEM when allocation fail? We can avoid to allocate memory
> again and again in suspend/resume cycles.

Yes, that would work.

Regards
Oliver


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

Re: [V3 PATCH 4/4] scsi:stex.c Add S3/S4 support

[Oliver Neukum]

On Wed, 2014-12-10 at 09:38 +0800, Charles Chiou wrote:
>  From 91868d4afe10533b8a4496075109e411100217bb Mon Sep 17 00:00:00 2001
> From: Charles Chiou 
> Date: Fri, 7 Nov 2014 10:15:18 +0800
> Subject: [PATCH 4/4] scsi:stex.c Add S3/S4 support
> 
> Add S3/S4 support, add .suspend and .resume function in pci_driver.
> 
> Pegasus need 30~40 seconds to boot up. We don't want to OS wait
> in .resume function. Create a thread to handle device boot up.
> 

> +static int stex_resume(struct pci_dev *pdev)
> +{
> + struct st_hba *hba = pci_get_drvdata(pdev);
> + struct hba_handshake_workstruct *hswork;
> + int sts;
> +
> + hba->mu_status = MU_STATE_STARTING;
> + hswork = kzalloc(sizeof(struct hba_handshake_workstruct), GFP_KERNEL);

The system is coming back from sleep. You cannot swap or page out
as disks may still be asleep. GFP_KERNEL is automatically changed
to GFP_NOIO. It would be nice to outright use GFP_NOIO.

> + INIT_WORK(&hswork->handshake_work, resume_handshake);

Memory allocations can fail.
I suggest you allocate the memory in suspend(). There you can just
return -ENOMEM in the error case.

Regards
Oliver



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

Re: [PATCH v10 2/6] backlight: Add support Skyworks SKY81452 backlight driver

[Oliver Neukum]

On Wed, 2014-12-03 at 16:05 +0900, gyun...@gmail.com wrote:
> +static ssize_t sky81452_bl_store_enable(struct device *dev,
> +   struct device_attribute *attr, const char *buf, size_t
> count)
> +{
> +   struct regmap *regmap = bl_get_data(to_backlight_device(dev));
> +   unsigned long value;
> +   int ret;
> +
> +   ret = kstrtoul(buf, 16, &value);
> +   if (IS_ERR_VALUE(ret))
> +   return ret;
> +
> +   ret = regmap_update_bits(regmap, SKY81452_REG1, SKY81452_EN,
> +   value << CTZ(SKY81452_EN));

No range checking for value?

> +   if (IS_ERR_VALUE(ret))
> +   return ret;
> +
> +   return count;
> +}

Regards
Oliver

-- 
Oliver Neukum 

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

Re: bluetooth related firmware loader spew on resume.

[Oliver Neukum]

On Wed, 2014-11-26 at 15:31 +0100, Takashi Iwai wrote:

> Yes, that's what I mentioned in my reply.  But, actually more puzzling
> is that the WARNING shouldn't have been triggered at all.  That is,
> something is still racy there, so we'd need to fix it.

Did it trigger before khubd was replaced by a work queue?

Regards
Oliver


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

Re: bluetooth related firmware loader spew on resume.

[Oliver Neukum]

On Wed, 2014-11-26 at 15:12 +0100, Takashi Iwai wrote:
> At Wed, 26 Nov 2014 23:05:20 +0900,
> Marcel Holtmann wrote:
> > 
> > Hi Oliver,
> > 
> > >> In order to paper over this, we may also remember the failing firmware
> > >> and avoid loading it.  This might be an easer way than the endless
> > >> fight against UMH race...
> > > 
> > > 
> > > the full fix would be to implement reset_resume() for btusb.
> > > It seems to me that setup() should be split in two methods,
> > > one to request the firmware from user space and the second
> > > to transfer it to the device. reset_resume() would just need
> > > to repeat the second operation.
> > 
> > so when you do hci_register_dev, then hdev->setup is only called once. I 
> > really mean only once per lifetime of the hci_dev. So you would need to 
> > unregister the hci_dev first before hdev->setup will ever be called again. 
> > So I am not sure this is actually the problem here. The problem here is 
> > entirely within request_firmware() unless of course we run through the USB 
> > probe handlers again. Which I do not see happening here.
> > 
> > And we have hdev->setup this way since normally the Bluetooth devices keep 
> > their firmware patches and not forget about them and suspend-resume cycles. 
> > If the USB device of course jumps of the bus during it then all bets are 
> > off anyway.
> 
> Usually you can avoid unnecessary rebinding when you provide a proper
> reset_resume callback.  I guess that's what Oliver suggested.

Yes, but even in reset_resume() you would need to redo the setup
part, as the device lost power. The basic problem of requesting
the firmware wouldn't be solved.

Regards
Oliver


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

Re: bluetooth related firmware loader spew on resume.

[Oliver Neukum]

On Wed, 2014-11-26 at 11:53 +0100, Takashi Iwai wrote:
> At Wed, 26 Nov 2014 11:43:36 +0100,
> Oliver Neukum wrote:
> > 
> > On Wed, 2014-11-26 at 11:31 +0100, Takashi Iwai wrote:
> > > At Wed, 26 Nov 2014 11:10:23 +0100,
> > > Oliver Neukum wrote:
> > > > 
> > > > On Wed, 2014-11-26 at 09:52 +0100, Takashi Iwai wrote:
> > > > > At Wed, 26 Nov 2014 14:15:27 +0900,
> > > > 
> > > > > In order to paper over this, we may also remember the failing firmware
> > > > > and avoid loading it.  This might be an easer way than the endless
> > > > > fight against UMH race...
> > > > 
> > > > Hi,
> > > > 
> > > > the full fix would be to implement reset_resume() for btusb.
> > > > It seems to me that setup() should be split in two methods,
> > > > one to request the firmware from user space and the second
> > > > to transfer it to the device. reset_resume() would just need
> > > > to repeat the second operation.
> > > 
> > > I'm not against it, but one slight drawback is that you'll have to
> > > remember the firmware content to transfer by the driver itself in this
> > > scenario.   In the firmware loader framework, the content is re-read
> > > at resume so that the largish content isn't kept unnecessarily during
> > > the whole operation. 
> > 
> > That isn't a drawback but an advantage. Firmware for devices that
> > do power management needs to be in RAM. The right time to free it
> > is in disconnect(). But why does that mean that the driver has to
> > manage the firmware? Can't the firmware layer do it?
> 
> The f/w loader remembers the f/w names of the successful loads, and
> retries to load them automatically at the suspend time.  But it
> doesn't remember/cache the failed loads.  So, when the driver retires

Two issues

1. the firmware may be added later. So we could only record the name,
not the result of the query.
2. a driver may query several firmwares in turn to find its firmware

It seems to me that a firmware that will be needed again should
just not be evicted from RAM.

> request_firmware() for a non-existing file in the resume path, it
> actually reaches to the f/w loading part again unexpectedly.

And that is probably a bug. We just cannot request a firmware during
resumption. On anything but a leave node it is potentially deadly.

Regards
Oliver


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

Re: bluetooth related firmware loader spew on resume.

[Oliver Neukum]

On Wed, 2014-11-26 at 23:05 +0900, Marcel Holtmann wrote:
> Hi Oliver,
> 
> >> In order to paper over this, we may also remember the failing firmware
> >> and avoid loading it.  This might be an easer way than the endless
> >> fight against UMH race...
> > 
> > 
> > the full fix would be to implement reset_resume() for btusb.
> > It seems to me that setup() should be split in two methods,
> > one to request the firmware from user space and the second
> > to transfer it to the device. reset_resume() would just need
> > to repeat the second operation.
> 
> so when you do hci_register_dev, then hdev->setup is only called once. I 
> really mean only once per lifetime of the hci_dev. So you would need to 
> unregister the hci_dev first before hdev->setup will ever be called again. So 
> I am not sure this is actually the problem here. The problem here is entirely 
> within request_firmware() unless of course we run through the USB probe 
> handlers again. Which I do not see happening here.

It seems most likely to me that probing is indeed done again.
btusb does not implement reset_resume(). If power goes away
as is usual on S3/4 the device is reenumerated. The original
trace has a call to btusb_setup_bcm_patchram().
What else could be happening?

Regards
Oliver



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

Re: bluetooth related firmware loader spew on resume.

[Oliver Neukum]

On Wed, 2014-11-26 at 11:31 +0100, Takashi Iwai wrote:
> At Wed, 26 Nov 2014 11:10:23 +0100,
> Oliver Neukum wrote:
> > 
> > On Wed, 2014-11-26 at 09:52 +0100, Takashi Iwai wrote:
> > > At Wed, 26 Nov 2014 14:15:27 +0900,
> > 
> > > In order to paper over this, we may also remember the failing firmware
> > > and avoid loading it.  This might be an easer way than the endless
> > > fight against UMH race...
> > 
> > Hi,
> > 
> > the full fix would be to implement reset_resume() for btusb.
> > It seems to me that setup() should be split in two methods,
> > one to request the firmware from user space and the second
> > to transfer it to the device. reset_resume() would just need
> > to repeat the second operation.
> 
> I'm not against it, but one slight drawback is that you'll have to
> remember the firmware content to transfer by the driver itself in this
> scenario.   In the firmware loader framework, the content is re-read
> at resume so that the largish content isn't kept unnecessarily during
> the whole operation. 

That isn't a drawback but an advantage. Firmware for devices that
do power management needs to be in RAM. The right time to free it
is in disconnect(). But why does that mean that the driver has to
manage the firmware? Can't the firmware layer do it?

You just cannot keep a device operational seamlessly if you request
firmware on resume. We could in theory use a notification queue
running while user space is operational if you really want to save
a little RAM.

Regards
Oliver


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

Re: bluetooth related firmware loader spew on resume.

[Oliver Neukum]

On Wed, 2014-11-26 at 09:52 +0100, Takashi Iwai wrote:
> At Wed, 26 Nov 2014 14:15:27 +0900,

> In order to paper over this, we may also remember the failing firmware
> and avoid loading it.  This might be an easer way than the endless
> fight against UMH race...

Hi,

the full fix would be to implement reset_resume() for btusb.
It seems to me that setup() should be split in two methods,
one to request the firmware from user space and the second
to transfer it to the device. reset_resume() would just need
to repeat the second operation.

Regards
Oliver



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

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Oliver Neukum]

On Mon, 2014-11-24 at 16:56 -0800, Benson Leung wrote:
> Hi Oliver,
> 
> On Mon, Nov 24, 2014 at 1:13 AM, Oliver Neukum  wrote:
> >
> > But there is very little to be gained by switching off remote wakeup.
> > The additional energy consumption devices with remote wakeup enabled
> > will be dwarfed by the energy needed for an additional wakeup.
> >
> 
> That makes sense to me. Does this mean we should be moving toward a
> solution that doesn't wake suspended devices on close for other usb
> devices, not just hid?
> 
> This particular pattern of get()/needs_remote_wakeup=0/put() on
> close() appears in several other drivers, for example :
> 62ecae0 Input: wacom - properly enable runtime PM
> 5d9efc5 Input: usbtouchscreen - implement runtime power management

Yes, we should never wake up a device just to unset remote
wakeup for runtime PM. In hindsight those patches were clumsy.

Regards
Oliver


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

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Oliver Neukum]

On Fri, 2014-11-21 at 17:00 -0800, Benson Leung wrote:

> If devices are already asleep with this flag enabled, that means that
> they are presently configured for remote wake.

Yes, but that doesn't matter. The drivers must be ready for a device
being resumed at any time. Remote wakeup just adds one more reason.

> Waking the device in the case of a close() is appropriate because it
> also has the effect of re-suspending the device with the capability
> disabled, as it is no longer necessary.

But there is very little to be gained by switching off remote wakeup.
The additional energy consumption devices with remote wakeup enabled
will be dwarfed by the energy needed for an additional wakeup.

Regards
Oliver


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