Re: [PATCH 2/2] gadget: Support for the usb charger framework

2015-08-08 Thread Baolin Wang
On 8 August 2015 at 01:53, Greg KH gre...@linuxfoundation.org wrote:
 On Fri, Aug 07, 2015 at 05:22:47PM +0800, Baolin Wang wrote:
 On 7 August 2015 at 17:07, Peter Chen peter.c...@freescale.com wrote:
 
/**
 * struct usb_udc - describes one usb device controller @@ -127,12
   +128,45 @@ void usb_gadget_giveback_request(struct usb_ep *ep,  }
   EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
  
   +int usb_gadget_register_notify(struct usb_gadget *gadget,
   +struct notifier_block *nb) {
   + unsigned long flags;
   + int ret;
   +
   + spin_lock_irqsave(gadget-lock, flags);
  
   I find you use so many spin_lock_irqsave, any reasons for that?
   Why mutex_lock can't be used?
  
 
  The spin_lock_irqsave() can make it as a atomic notifier, that can make 
  sure the
  gadget state event can be quickly reported to the user who register a 
  notifier
  on the gadget device. Is it OK?
 
 
  I don't think it is a good reason, spin_lock_irqsave is usually used for 
  protecting
  data which is accessed at atomic environment.
 

 Yes, we want the notify process is a atomic environment which do not
 want to be interrupted by irq or other things to make the notice can
 be quickly reported to the user.

 No, this is a slow event, you don't need to notify anyone under atomic
 context, that's crazy.

 Also i think the notify process is less cost, thus i use the spinlock. 
 Thanks.

 No, use a mutex please, that's the safe thing.  This is not
 time-critical code at all.


OK, Thanks for your comments and will fix the lock thing.

 thanks,

 greg k-h



-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [PATCH 2/2] gadget: Support for the usb charger framework

2015-08-07 Thread Peter Chen
 
   /**
* struct usb_udc - describes one usb device controller @@ -127,12
  +128,45 @@ void usb_gadget_giveback_request(struct usb_ep *ep,  }
  EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
 
  +int usb_gadget_register_notify(struct usb_gadget *gadget,
  +struct notifier_block *nb) {
  + unsigned long flags;
  + int ret;
  +
  + spin_lock_irqsave(gadget-lock, flags);
 
  I find you use so many spin_lock_irqsave, any reasons for that?
  Why mutex_lock can't be used?
 
 
 The spin_lock_irqsave() can make it as a atomic notifier, that can make sure 
 the
 gadget state event can be quickly reported to the user who register a notifier
 on the gadget device. Is it OK?
 

I don't think it is a good reason, spin_lock_irqsave is usually used for 
protecting
data which is accessed at atomic environment. 

Peter


Re: [PATCH 2/2] gadget: Support for the usb charger framework

2015-08-07 Thread Peter Chen
On Thu, Aug 06, 2015 at 03:03:49PM +0800, Baolin Wang wrote:
 The usb charger framework is based on usb gadget, and each usb gadget
 can be one usb charger to set the current limitation.
 
 This patch adds a notifier mechanism for usb charger to report to usb
 charger when the usb gadget state is changed.
 
 Also we introduce a callback 'get_charger_type' which will implemented
 by user for usb gadget operations to get the usb charger type.
 
 Signed-off-by: Baolin Wang baolin.w...@linaro.org
 ---
  drivers/usb/gadget/udc/udc-core.c |   41 
 +
  include/linux/usb/gadget.h|   20 ++
  2 files changed, 61 insertions(+)
 
 diff --git a/drivers/usb/gadget/udc/udc-core.c 
 b/drivers/usb/gadget/udc/udc-core.c
 index d69c355..d5368088 100644
 --- a/drivers/usb/gadget/udc/udc-core.c
 +++ b/drivers/usb/gadget/udc/udc-core.c
 @@ -28,6 +28,7 @@
  #include linux/usb/ch9.h
  #include linux/usb/gadget.h
  #include linux/usb.h
 +#include linux/usb/usb_charger.h
  
  /**
   * struct usb_udc - describes one usb device controller
 @@ -127,12 +128,45 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
  }
  EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
  
 +int usb_gadget_register_notify(struct usb_gadget *gadget,
 +struct notifier_block *nb)
 +{
 + unsigned long flags;
 + int ret;
 +
 + spin_lock_irqsave(gadget-lock, flags);

I find you use so many spin_lock_irqsave, any reasons for that?
Why mutex_lock can't be used?

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] gadget: Support for the usb charger framework

2015-08-07 Thread Baolin Wang
On 7 August 2015 at 13:45, Peter Chen peter.c...@freescale.com wrote:
 On Thu, Aug 06, 2015 at 03:03:49PM +0800, Baolin Wang wrote:
 The usb charger framework is based on usb gadget, and each usb gadget
 can be one usb charger to set the current limitation.

 This patch adds a notifier mechanism for usb charger to report to usb
 charger when the usb gadget state is changed.

 Also we introduce a callback 'get_charger_type' which will implemented
 by user for usb gadget operations to get the usb charger type.

 Signed-off-by: Baolin Wang baolin.w...@linaro.org
 ---
  drivers/usb/gadget/udc/udc-core.c |   41 
 +
  include/linux/usb/gadget.h|   20 ++
  2 files changed, 61 insertions(+)

 diff --git a/drivers/usb/gadget/udc/udc-core.c 
 b/drivers/usb/gadget/udc/udc-core.c
 index d69c355..d5368088 100644
 --- a/drivers/usb/gadget/udc/udc-core.c
 +++ b/drivers/usb/gadget/udc/udc-core.c
 @@ -28,6 +28,7 @@
  #include linux/usb/ch9.h
  #include linux/usb/gadget.h
  #include linux/usb.h
 +#include linux/usb/usb_charger.h

  /**
   * struct usb_udc - describes one usb device controller
 @@ -127,12 +128,45 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
  }
  EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);

 +int usb_gadget_register_notify(struct usb_gadget *gadget,
 +struct notifier_block *nb)
 +{
 + unsigned long flags;
 + int ret;
 +
 + spin_lock_irqsave(gadget-lock, flags);

 I find you use so many spin_lock_irqsave, any reasons for that?
 Why mutex_lock can't be used?


The spin_lock_irqsave() can make it as a atomic notifier, that can
make sure the gadget state event can be quickly reported to the user
who register a notifier on the gadget device. Is it OK?

 --

 Best Regards,
 Peter Chen



-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] gadget: Support for the usb charger framework

2015-08-07 Thread Baolin Wang
On 7 August 2015 at 17:07, Peter Chen peter.c...@freescale.com wrote:

   /**
* struct usb_udc - describes one usb device controller @@ -127,12
  +128,45 @@ void usb_gadget_giveback_request(struct usb_ep *ep,  }
  EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
 
  +int usb_gadget_register_notify(struct usb_gadget *gadget,
  +struct notifier_block *nb) {
  + unsigned long flags;
  + int ret;
  +
  + spin_lock_irqsave(gadget-lock, flags);
 
  I find you use so many spin_lock_irqsave, any reasons for that?
  Why mutex_lock can't be used?
 

 The spin_lock_irqsave() can make it as a atomic notifier, that can make sure 
 the
 gadget state event can be quickly reported to the user who register a 
 notifier
 on the gadget device. Is it OK?


 I don't think it is a good reason, spin_lock_irqsave is usually used for 
 protecting
 data which is accessed at atomic environment.


Yes, we want the notify process is a atomic environment which do not
want to be interrupted by irq or other things to make the notice can
be quickly reported to the user.

Also i think the notify process is less cost, thus i use the spinlock. Thanks.

 Peter



-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] gadget: Support for the usb charger framework

2015-08-07 Thread Greg KH
On Fri, Aug 07, 2015 at 05:22:47PM +0800, Baolin Wang wrote:
 On 7 August 2015 at 17:07, Peter Chen peter.c...@freescale.com wrote:
 
/**
 * struct usb_udc - describes one usb device controller @@ -127,12
   +128,45 @@ void usb_gadget_giveback_request(struct usb_ep *ep,  }
   EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
  
   +int usb_gadget_register_notify(struct usb_gadget *gadget,
   +struct notifier_block *nb) {
   + unsigned long flags;
   + int ret;
   +
   + spin_lock_irqsave(gadget-lock, flags);
  
   I find you use so many spin_lock_irqsave, any reasons for that?
   Why mutex_lock can't be used?
  
 
  The spin_lock_irqsave() can make it as a atomic notifier, that can make 
  sure the
  gadget state event can be quickly reported to the user who register a 
  notifier
  on the gadget device. Is it OK?
 
 
  I don't think it is a good reason, spin_lock_irqsave is usually used for 
  protecting
  data which is accessed at atomic environment.
 
 
 Yes, we want the notify process is a atomic environment which do not
 want to be interrupted by irq or other things to make the notice can
 be quickly reported to the user.

No, this is a slow event, you don't need to notify anyone under atomic
context, that's crazy.

 Also i think the notify process is less cost, thus i use the spinlock. Thanks.

No, use a mutex please, that's the safe thing.  This is not
time-critical code at all.

thanks,

greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html