Re: [PATCH V2 1/1] staging: usbip: remove an unnecessary lock inusbip_event_happened.

[Greg Kroah-Hartman]

On Mon, Nov 05, 2012 at 05:51:11PM +0800, harvey.yang wrote:
> From: Harvey.Yang <harvey.huawei.yang@xxxxxxxxx>
> 
> 
> Signed-off-by: Harvey.Yang <harvey.huawei.yang@xxxxxxxxx>
> ---
>  drivers/staging/usbip/usbip_common.h |    2 +-
>  drivers/staging/usbip/usbip_event.c  |   11 ++---------
>  2 files changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/usbip/usbip_common.h b/drivers/staging/usbip/usbip_common.h
> index 5d89c0f..b6d7add 100644
> --- a/drivers/staging/usbip/usbip_common.h
> +++ b/drivers/staging/usbip/usbip_common.h
> @@ -330,7 +330,7 @@ int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb);
>  int usbip_start_eh(struct usbip_device *ud);
>  void usbip_stop_eh(struct usbip_device *ud);
>  void usbip_event_add(struct usbip_device *ud, unsigned long event);
> -int usbip_event_happened(struct usbip_device *ud);
> +inline int usbip_event_happened(struct usbip_device *ud);
>  
>  static inline int interface_to_busnum(struct usb_interface *interface)
>  {
> diff --git a/drivers/staging/usbip/usbip_event.c b/drivers/staging/usbip/usbip_event.c
> index d332a34..15871f0 100644
> --- a/drivers/staging/usbip/usbip_event.c
> +++ b/drivers/staging/usbip/usbip_event.c
> @@ -112,15 +112,8 @@ void usbip_event_add(struct usbip_device *ud, unsigned long event)
>  }
>  EXPORT_SYMBOL_GPL(usbip_event_add);
>  
> -int usbip_event_happened(struct usbip_device *ud)
> +inline int usbip_event_happened(struct usbip_device *ud)
>  {
> -	int happened = 0;
> -
> -	spin_lock(&ud->lock);
> -	if (ud->event != 0)
> -		happened = 1;
> -	spin_unlock(&ud->lock);
> -
> -	return happened;
> +	return ud->event ? 1 : 0;

No, you aren't locking to see if something changed with ud->event here.

Actually, that's pretty broken anyway, what is to say that event doesn't
change right after this call?  What exactly is this call supposed to be
doing?  Who is calling it?

>  }
>  EXPORT_SYMBOL_GPL(usbip_event_happened);

You can't inline an exported symbol, how would that work?

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