diff mbox series

[01/11] usb: gadget: add anonymous definition in struct usb_gadget

Message ID 20230911042843.2711-2-quic_linyyuan@quicinc.com (mailing list archive)
State New, archived
Headers show
Series reduce usb gadget trace event buffer usage | expand

Commit Message

Linyu Yuan Sept. 11, 2023, 4:28 a.m. UTC
Some UDC trace event will save usb gadget information, but it will use
one int size buffer to save one bit information of usb gadget, so more
than one int buffer to save several bit fields which is not good.

Add one anonymous union have three u32 members which can be used by trace
event during fast assign stage to reduce trace buffer usage, and add
related macro to extract bit fields from u32 members for later trace event
output state usage.

Also move sg_supported and other bit fields into one anonymous struct
which inside anonymous union and Change bit fields from unsigned to u32
type, it will make sure union member have expected u32 size.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
 include/linux/usb/gadget.h | 63 ++++++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 19 deletions(-)

Comments

Alan Stern Sept. 11, 2023, 2:53 p.m. UTC | #1
On Mon, Sep 11, 2023 at 12:28:33PM +0800, Linyu Yuan wrote:
> Some UDC trace event will save usb gadget information, but it will use
> one int size buffer to save one bit information of usb gadget, so more
> than one int buffer to save several bit fields which is not good.
> 
> Add one anonymous union have three u32 members which can be used by trace
> event during fast assign stage to reduce trace buffer usage, and add
> related macro to extract bit fields from u32 members for later trace event
> output state usage.
> 
> Also move sg_supported and other bit fields into one anonymous struct
> which inside anonymous union and Change bit fields from unsigned to u32
> type, it will make sure union member have expected u32 size.
> 
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
>  include/linux/usb/gadget.h | 63 ++++++++++++++++++++++++++------------
>  1 file changed, 44 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 75bda0783395..cdf62e7f34e7 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -357,6 +357,7 @@ struct usb_gadget_ops {
>   * @in_epnum: last used in ep number
>   * @mA: last set mA value
>   * @otg_caps: OTG capabilities of this gadget.
> + * @dw1: trace event purpose
>   * @sg_supported: true if we can handle scatter-gather
>   * @is_otg: True if the USB device port uses a Mini-AB jack, so that the
>   *	gadget driver must provide a USB OTG descriptor.
> @@ -432,25 +433,49 @@ struct usb_gadget {
>  	unsigned			mA;
>  	struct usb_otg_caps		*otg_caps;
>  
> -	unsigned			sg_supported:1;
> -	unsigned			is_otg:1;
> -	unsigned			is_a_peripheral:1;
> -	unsigned			b_hnp_enable:1;
> -	unsigned			a_hnp_support:1;
> -	unsigned			a_alt_hnp_support:1;
> -	unsigned			hnp_polling_support:1;
> -	unsigned			host_request_flag:1;
> -	unsigned			quirk_ep_out_aligned_size:1;
> -	unsigned			quirk_altset_not_supp:1;
> -	unsigned			quirk_stall_not_supp:1;
> -	unsigned			quirk_zlp_not_supp:1;
> -	unsigned			quirk_avoids_skb_reserve:1;
> -	unsigned			is_selfpowered:1;
> -	unsigned			deactivated:1;
> -	unsigned			connected:1;
> -	unsigned			lpm_capable:1;
> -	unsigned			wakeup_capable:1;
> -	unsigned			wakeup_armed:1;
> +	union {
> +		struct {
> +			u32		sg_supported:1;
> +			u32		is_otg:1;
> +			u32		is_a_peripheral:1;
> +			u32		b_hnp_enable:1;
> +			u32		a_hnp_support:1;
> +			u32		a_alt_hnp_support:1;
> +			u32		hnp_polling_support:1;
> +			u32		host_request_flag:1;
> +			u32		quirk_ep_out_aligned_size:1;
> +			u32		quirk_altset_not_supp:1;
> +			u32		quirk_stall_not_supp:1;
> +			u32		quirk_zlp_not_supp:1;
> +			u32		quirk_avoids_skb_reserve:1;
> +			u32		is_selfpowered:1;
> +			u32		deactivated:1;
> +			u32		connected:1;
> +			u32		lpm_capable:1;
> +			u32		wakeup_capable:1;
> +			u32		wakeup_armed:1;
> +		} __packed;
> +		u32			dw1;
> +#define		USB_GADGET_SG_SUPPORTED(n)			((n) & BIT(0))
> +#define		USB_GADGET_IS_OTG(n)				((n) & BIT(1))
> +#define		USB_GADGET_IS_A_PERIPHERAL(n)			((n) & BIT(2))
> +#define		USB_GADGET_B_HNP_ENABLE(n)			((n) & BIT(3))
> +#define		USB_GADGET_A_HNP_SUPPORT(n)			((n) & BIT(4))
> +#define		USB_GADGET_A_ALT_HNP_SUPPORT(n)			((n) & BIT(5))
> +#define		USB_GADGET_HNP_POLLING_SUPPORT(n)		((n) & BIT(6))
> +#define		USB_GADGET_HOST_REQUEST_FLAG(n)			((n) & BIT(7))
> +#define		USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE(n)		((n) & BIT(8))
> +#define		USB_GADGET_QUIRK_ALTSET_NOT_SUPP(n)		((n) & BIT(9))
> +#define		USB_GADGET_QUIRK_STALL_NOT_SUPP(n)		((n) & BIT(10))
> +#define		USB_GADGET_QUIRK_ZLP_NOT_SUPP(n)		((n) & BIT(11))
> +#define		USB_GADGET_QUIRK_AVOIDS_SKB_RESERVE(n)		((n) & BIT(12))
> +#define		USB_GADGET_IS_SELFPOWERED(n)			((n) & BIT(13))
> +#define		USB_GADGET_DEACTIVATED(n)			((n) & BIT(14))
> +#define		USB_GADGET_CONNECTED(n)				((n) & BIT(15))
> +#define		USB_GADGET_LPM_CAPABLE(n)			((n) & BIT(16))
> +#define		USB_GADGET_WAKEUP_CAPABLE(n)			((n) & BIT(17))
> +#define		USB_GADGET_WAKEUP_ARMED(n)			((n) & BIT(18))
> +	};

Are you sure these #defines will work on all architectures?  As far as I 
know, the C compiler does not specify that bit fields in packed 
structures will be assigned starting from the low-order bit position.

Alan Stern
Linyu Yuan Sept. 12, 2023, 12:16 a.m. UTC | #2
On 9/11/2023 10:53 PM, Alan Stern wrote:
> On Mon, Sep 11, 2023 at 12:28:33PM +0800, Linyu Yuan wrote:
>> Some UDC trace event will save usb gadget information, but it will use
>> one int size buffer to save one bit information of usb gadget, so more
>> than one int buffer to save several bit fields which is not good.
>>
>> Add one anonymous union have three u32 members which can be used by trace
>> event during fast assign stage to reduce trace buffer usage, and add
>> related macro to extract bit fields from u32 members for later trace event
>> output state usage.
>>
>> Also move sg_supported and other bit fields into one anonymous struct
>> which inside anonymous union and Change bit fields from unsigned to u32
>> type, it will make sure union member have expected u32 size.
>>
>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>> ---
>>   include/linux/usb/gadget.h | 63 ++++++++++++++++++++++++++------------
>>   1 file changed, 44 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index 75bda0783395..cdf62e7f34e7 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -357,6 +357,7 @@ struct usb_gadget_ops {
>>    * @in_epnum: last used in ep number
>>    * @mA: last set mA value
>>    * @otg_caps: OTG capabilities of this gadget.
>> + * @dw1: trace event purpose
>>    * @sg_supported: true if we can handle scatter-gather
>>    * @is_otg: True if the USB device port uses a Mini-AB jack, so that the
>>    *	gadget driver must provide a USB OTG descriptor.
>> @@ -432,25 +433,49 @@ struct usb_gadget {
>>   	unsigned			mA;
>>   	struct usb_otg_caps		*otg_caps;
>>   
>> -	unsigned			sg_supported:1;
>> -	unsigned			is_otg:1;
>> -	unsigned			is_a_peripheral:1;
>> -	unsigned			b_hnp_enable:1;
>> -	unsigned			a_hnp_support:1;
>> -	unsigned			a_alt_hnp_support:1;
>> -	unsigned			hnp_polling_support:1;
>> -	unsigned			host_request_flag:1;
>> -	unsigned			quirk_ep_out_aligned_size:1;
>> -	unsigned			quirk_altset_not_supp:1;
>> -	unsigned			quirk_stall_not_supp:1;
>> -	unsigned			quirk_zlp_not_supp:1;
>> -	unsigned			quirk_avoids_skb_reserve:1;
>> -	unsigned			is_selfpowered:1;
>> -	unsigned			deactivated:1;
>> -	unsigned			connected:1;
>> -	unsigned			lpm_capable:1;
>> -	unsigned			wakeup_capable:1;
>> -	unsigned			wakeup_armed:1;
>> +	union {
>> +		struct {
>> +			u32		sg_supported:1;
>> +			u32		is_otg:1;
>> +			u32		is_a_peripheral:1;
>> +			u32		b_hnp_enable:1;
>> +			u32		a_hnp_support:1;
>> +			u32		a_alt_hnp_support:1;
>> +			u32		hnp_polling_support:1;
>> +			u32		host_request_flag:1;
>> +			u32		quirk_ep_out_aligned_size:1;
>> +			u32		quirk_altset_not_supp:1;
>> +			u32		quirk_stall_not_supp:1;
>> +			u32		quirk_zlp_not_supp:1;
>> +			u32		quirk_avoids_skb_reserve:1;
>> +			u32		is_selfpowered:1;
>> +			u32		deactivated:1;
>> +			u32		connected:1;
>> +			u32		lpm_capable:1;
>> +			u32		wakeup_capable:1;
>> +			u32		wakeup_armed:1;
>> +		} __packed;
>> +		u32			dw1;
>> +#define		USB_GADGET_SG_SUPPORTED(n)			((n) & BIT(0))
>> +#define		USB_GADGET_IS_OTG(n)				((n) & BIT(1))
>> +#define		USB_GADGET_IS_A_PERIPHERAL(n)			((n) & BIT(2))
>> +#define		USB_GADGET_B_HNP_ENABLE(n)			((n) & BIT(3))
>> +#define		USB_GADGET_A_HNP_SUPPORT(n)			((n) & BIT(4))
>> +#define		USB_GADGET_A_ALT_HNP_SUPPORT(n)			((n) & BIT(5))
>> +#define		USB_GADGET_HNP_POLLING_SUPPORT(n)		((n) & BIT(6))
>> +#define		USB_GADGET_HOST_REQUEST_FLAG(n)			((n) & BIT(7))
>> +#define		USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE(n)		((n) & BIT(8))
>> +#define		USB_GADGET_QUIRK_ALTSET_NOT_SUPP(n)		((n) & BIT(9))
>> +#define		USB_GADGET_QUIRK_STALL_NOT_SUPP(n)		((n) & BIT(10))
>> +#define		USB_GADGET_QUIRK_ZLP_NOT_SUPP(n)		((n) & BIT(11))
>> +#define		USB_GADGET_QUIRK_AVOIDS_SKB_RESERVE(n)		((n) & BIT(12))
>> +#define		USB_GADGET_IS_SELFPOWERED(n)			((n) & BIT(13))
>> +#define		USB_GADGET_DEACTIVATED(n)			((n) & BIT(14))
>> +#define		USB_GADGET_CONNECTED(n)				((n) & BIT(15))
>> +#define		USB_GADGET_LPM_CAPABLE(n)			((n) & BIT(16))
>> +#define		USB_GADGET_WAKEUP_CAPABLE(n)			((n) & BIT(17))
>> +#define		USB_GADGET_WAKEUP_ARMED(n)			((n) & BIT(18))
>> +	};
> Are you sure these #defines will work on all architectures?  As far as I
> know, the C compiler does not specify that bit fields in packed
> structures will be assigned starting from the low-order bit position.


thanks for review, will make it more generic for little/big endian and 
your concern.


>
> Alan Stern
diff mbox series

Patch

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 75bda0783395..cdf62e7f34e7 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -357,6 +357,7 @@  struct usb_gadget_ops {
  * @in_epnum: last used in ep number
  * @mA: last set mA value
  * @otg_caps: OTG capabilities of this gadget.
+ * @dw1: trace event purpose
  * @sg_supported: true if we can handle scatter-gather
  * @is_otg: True if the USB device port uses a Mini-AB jack, so that the
  *	gadget driver must provide a USB OTG descriptor.
@@ -432,25 +433,49 @@  struct usb_gadget {
 	unsigned			mA;
 	struct usb_otg_caps		*otg_caps;
 
-	unsigned			sg_supported:1;
-	unsigned			is_otg:1;
-	unsigned			is_a_peripheral:1;
-	unsigned			b_hnp_enable:1;
-	unsigned			a_hnp_support:1;
-	unsigned			a_alt_hnp_support:1;
-	unsigned			hnp_polling_support:1;
-	unsigned			host_request_flag:1;
-	unsigned			quirk_ep_out_aligned_size:1;
-	unsigned			quirk_altset_not_supp:1;
-	unsigned			quirk_stall_not_supp:1;
-	unsigned			quirk_zlp_not_supp:1;
-	unsigned			quirk_avoids_skb_reserve:1;
-	unsigned			is_selfpowered:1;
-	unsigned			deactivated:1;
-	unsigned			connected:1;
-	unsigned			lpm_capable:1;
-	unsigned			wakeup_capable:1;
-	unsigned			wakeup_armed:1;
+	union {
+		struct {
+			u32		sg_supported:1;
+			u32		is_otg:1;
+			u32		is_a_peripheral:1;
+			u32		b_hnp_enable:1;
+			u32		a_hnp_support:1;
+			u32		a_alt_hnp_support:1;
+			u32		hnp_polling_support:1;
+			u32		host_request_flag:1;
+			u32		quirk_ep_out_aligned_size:1;
+			u32		quirk_altset_not_supp:1;
+			u32		quirk_stall_not_supp:1;
+			u32		quirk_zlp_not_supp:1;
+			u32		quirk_avoids_skb_reserve:1;
+			u32		is_selfpowered:1;
+			u32		deactivated:1;
+			u32		connected:1;
+			u32		lpm_capable:1;
+			u32		wakeup_capable:1;
+			u32		wakeup_armed:1;
+		} __packed;
+		u32			dw1;
+#define		USB_GADGET_SG_SUPPORTED(n)			((n) & BIT(0))
+#define		USB_GADGET_IS_OTG(n)				((n) & BIT(1))
+#define		USB_GADGET_IS_A_PERIPHERAL(n)			((n) & BIT(2))
+#define		USB_GADGET_B_HNP_ENABLE(n)			((n) & BIT(3))
+#define		USB_GADGET_A_HNP_SUPPORT(n)			((n) & BIT(4))
+#define		USB_GADGET_A_ALT_HNP_SUPPORT(n)			((n) & BIT(5))
+#define		USB_GADGET_HNP_POLLING_SUPPORT(n)		((n) & BIT(6))
+#define		USB_GADGET_HOST_REQUEST_FLAG(n)			((n) & BIT(7))
+#define		USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE(n)		((n) & BIT(8))
+#define		USB_GADGET_QUIRK_ALTSET_NOT_SUPP(n)		((n) & BIT(9))
+#define		USB_GADGET_QUIRK_STALL_NOT_SUPP(n)		((n) & BIT(10))
+#define		USB_GADGET_QUIRK_ZLP_NOT_SUPP(n)		((n) & BIT(11))
+#define		USB_GADGET_QUIRK_AVOIDS_SKB_RESERVE(n)		((n) & BIT(12))
+#define		USB_GADGET_IS_SELFPOWERED(n)			((n) & BIT(13))
+#define		USB_GADGET_DEACTIVATED(n)			((n) & BIT(14))
+#define		USB_GADGET_CONNECTED(n)				((n) & BIT(15))
+#define		USB_GADGET_LPM_CAPABLE(n)			((n) & BIT(16))
+#define		USB_GADGET_WAKEUP_CAPABLE(n)			((n) & BIT(17))
+#define		USB_GADGET_WAKEUP_ARMED(n)			((n) & BIT(18))
+	};
 	int				irq;
 	int				id_number;
 };