: use get/put_unaligned_* helpers to fix more potential unaligned issues.
Subsystems (3)
PATCH 1/1 : use get/put_unaligned_* helpers to fix more potential unaligned issues.
--- drivers/usb/host/isp116x-hcd.c | 4 ++-- drivers/usb/host/uhci-hub.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-)
drivers/usb/host/isp116x-hcd.c +2 -2
@@ -1024,7 +1024,7 @@ static int isp116x_hub_control(struct usb_hcd *hcd,break ;case GetHubStatus:DBG ("GetHubStatus\n" ); - *(__le32 *) buf =0 ; +put_unaligned_le32 (0 , buf);break ;case GetPortStatus:DBG ("GetPortStatus\n" );
@@ -1033,7 +1033,7 @@ static int isp116x_hub_control(struct usb_hcd *hcd,spin_lock_irqsave (&isp116x->lock, flags); tmp =isp116x_read_reg32 (isp116x, (--wIndex) ?HCRHPORT2 :HCRHPORT1 );spin_unlock_irqrestore (&isp116x->lock, flags); - *(__le32 *) buf =cpu_to_le32 (tmp); +put_unaligned_le32 (tmp, buf);DBG ("GetPortStatus: port[%d] %08x\n" , wIndex +1 , tmp);break ;case ClearPortFeature:
drivers/usb/host/uhci-hub.c +3 -3
@@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,switch (typeReq) {case GetHubStatus: - *(__le32 *)buf =cpu_to_le32 (0 ); +put_unaligned_le32 (0 , buf);OK (4 );/* hub power */ case GetPortStatus:if (port >= uhci->rh_numports)
@@ -306,8 +306,8 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,dev_dbg (uhci_dev (uhci),"port %d portsc %04x,%02x\n" , wIndex, status, lstatus); - *(__le16 *)buf =cpu_to_le16 (wPortStatus); - *(__le16 *)(buf +2 ) =cpu_to_le16 (wPortChange); +put_unaligned_le16 (wPortStatus, buf); +put_unaligned_le16 (wPortChange, buf +2 );OK (4 );case SetHubFeature:/* We don't implement these */ case ClearHubFeature:
Signature · 1 line
1.5.5
Am Donnerstag 15 Mai 2008 08:19:24 schrieb Bryan Wu:> --- a/drivers/usb/host/uhci-hub.c> +++ b/drivers/usb/host/uhci-hub.c> @@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,> switch (typeReq) {> > case GetHubStatus:> - *(__le32 *)buf = cpu_to_le32(0);> + put_unaligned_le32(0, buf); What is supposed to make all these changes a good idea?
Regards Oliver
���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m����������?�I�
Quoted 6 lines · Oliver Neukum
Am Donnerstag 15 Mai 2008 08:19:24 schrieb Bryan Wu:
Quoted 5 lines
drivers/usb/host/uhci-hub.c +0
-0@@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,switch (typeReq) {
Quoted 6 lines
drivers/usb/host/uhci-hub.c static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, · line 256
Quoted 3 lines
drivers/usb/host/uhci-hub.c +1
-1case GetHubStatus: - *(__le32 *)buf =cpu_to_le32 (0 ); +put_unaligned_le32 (0 , buf);What is supposed to make all these changes a good idea?
Since buf might not be 4-byte aligned.
Jie
Quoted 15 lines · Jie Zhang
Quoted 14 lines · Oliver Neukum
Am Donnerstag 15 Mai 2008 08:19:24 schrieb Bryan Wu:
drivers/usb/host/uhci-hub.c static int uhci_hub_control(struct usb_hcd *hcd, u16 · line 256
Quoted 9 lines
drivers/usb/host/uhci-hub.c +1
-1@@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, switch (typeReq) {case GetHubStatus: - *(__le32 *)buf =cpu_to_le32 (0 ); +put_unaligned_le32 (0 , buf);What is supposed to make all these changes a good idea?
Quoted 2 lines
Since buf might not be 4-byte aligned.
And this change follows Harvey Harrison's patch's style:
-Bryan
Am Donnerstag 15 Mai 2008 10:03:35 schrieb Jie Zhang:
Quoted 7 lines
Quoted 6 lines · Oliver Neukum
Am Donnerstag 15 Mai 2008 08:19:24 schrieb Bryan Wu:
Quoted 5 lines
drivers/usb/host/uhci-hub.c +0
-0@@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,switch (typeReq) {
Quoted 7 lines
Quoted 6 lines
drivers/usb/host/uhci-hub.c static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, · line 256
Quoted 3 lines
drivers/usb/host/uhci-hub.c +1
-1case GetHubStatus: - *(__le32 *)buf =cpu_to_le32 (0 ); +put_unaligned_le32 (0 , buf);What is supposed to make all these changes a good idea?
Since buf might not be 4-byte aligned.
It is. Please analyze the code before you use these access methods.
Regards
Oliver
Quoted 8 lines · Oliver Neukum
Am Donnerstag 15 Mai 2008 10:03:35 schrieb Jie Zhang:
Quoted 7 lines
Quoted 6 lines · Oliver Neukum
Am Donnerstag 15 Mai 2008 08:19:24 schrieb Bryan Wu:
Quoted 5 lines
drivers/usb/host/uhci-hub.c +0
-0@@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,switch (typeReq) {
Quoted 9 lines
Quoted 6 lines
Quoted 5 lines
drivers/usb/host/uhci-hub.c static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, · line 256
Quoted 3 lines
drivers/usb/host/uhci-hub.c +1
-1case GetHubStatus: - *(__le32 *)buf =cpu_to_le32 (0 ); +put_unaligned_le32 (0 , buf);What is supposed to make all these changes a good idea?
Since buf might not be 4-byte aligned.
It is. Please analyze the code before you use these access methods.
You are right. buf has been 4-byte aligned since 2.6.19. My patch was written two years ago. Sorry for the noise I caused.
Jie
Quoted 19 lines · Jie Zhang
Quoted 18 lines · Oliver Neukum
Am Donnerstag 15 Mai 2008 10:03:35 schrieb Jie Zhang:
Quoted 16 lines
Quoted 14 lines · Oliver Neukum
Am Donnerstag 15 Mai 2008 08:19:24 schrieb Bryan Wu:
drivers/usb/host/uhci-hub.c static int uhci_hub_control(struct usb_hcd *hcd, · line 256
Quoted 9 lines
drivers/usb/host/uhci-hub.c +1
-1@@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, switch (typeReq) { case GetHubStatus: - *(__le32 *)buf = cpu_to_le32(0); + put_unaligned_le32(0, buf);What is supposed to make all these changes a good idea?
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Since buf might not be 4-byte aligned.
It is. Please analyze the code before you use these access methods.
You are right. buf has been 4-byte aligned since 2.6.19. My patch was written two years ago. Sorry for the noise I caused.
I has keeping this patch for a long time. Jie fixed this patch at 2006/09/20 in our svn:
http://blackfin.uclinux.org/git/?p=readonly-mirrors/linux-kernel.git;a=commit;h=cb3da1243f84b37b53486c7e86da34565b4c5d92
http://blackfin.uclinux.org/git/?p=readonly-mirrors/linux-kernel.git;a=commit;h=2559298f0dca2cffc0b87390b92a484004f0d85e
And a similar patch from David Miller was accept in ohci-hub.c
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=92164c5dd1ade33f4e90b72e407910de6694de49
Also because of the same issue, which was fixed by:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=54bee6e1b455573658972510a76119f279db32b7
If other functions not only rh_call_control() call this hub_control() pointer and the buf is not 4-byte aligned,
this bug will fire again without the unaligned API. This patch is safer for the caller, although not efficient.
-Bryan
Am Donnerstag 15 Mai 2008 11:54:51 schrieb Bryan Wu:
Quoted 36 lines · Jie Zhang
Quoted 19 lines
Quoted 18 lines · Oliver Neukum
Am Donnerstag 15 Mai 2008 10:03:35 schrieb Jie Zhang:
Quoted 16 lines
Quoted 14 lines · Oliver Neukum
Am Donnerstag 15 Mai 2008 08:19:24 schrieb Bryan Wu:
drivers/usb/host/uhci-hub.c static int uhci_hub_control(struct usb_hcd *hcd, · line 256
Quoted 9 lines
drivers/usb/host/uhci-hub.c +1
-1@@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, switch (typeReq) { case GetHubStatus: - *(__le32 *)buf = cpu_to_le32(0); + put_unaligned_le32(0, buf);What is supposed to make all these changes a good idea?
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Since buf might not be 4-byte aligned.
It is. Please analyze the code before you use these access methods.
You are right. buf has been 4-byte aligned since 2.6.19. My patch was written two years ago. Sorry for the noise I caused.
I has keeping this patch for a long time. Jie fixed this patch at 2006/09/20 in our svn:
http://blackfin.uclinux.org/git/?p=readonly-mirrors/linux-kernel.git;a=commit;h=cb3da1243f84b37b53486c7e86da34565b4c5d92
http://blackfin.uclinux.org/git/?p=readonly-mirrors/linux-kernel.git;a=commit;h=2559298f0dca2cffc0b87390b92a484004f0d85eAnd a similar patch from David Miller was accept in
ohci-hub.c
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=92164c5dd1ade33f4e90b72e407910de6694de49
Quoted 7 lines
Also because of the same issue, which was fixed by:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=54bee6e1b455573658972510a76119f279db32b7If other functions not only
rh_call_control()call thishub_control()pointer and the buf is not 4-byte aligned,
this bug will fire again without the unaligned API. This patch is safer for the caller, although not efficient.
This is not very nice. If we pass around unaligned pointers we should mark those not catch errors later on.
Regards
Oliver
Quoted 2 lines · Oliver Neukum
This is not very nice. If we pass around unaligned pointers we should mark those not catch errors later on.
ISTR that GCC doesn't have a way to annotate pointers that way.
Am Donnerstag 15 Mai 2008 15:03:20 schrieben Sie:
Quoted 5 lines · Oliver Neukum
Quoted 2 lines
This is not very nice. If we pass around unaligned pointers we should mark those not catch errors later on.
ISTR that GCC doesn't have a way to annotate pointers that way.
But we can do the _user annotation, can't we?
Regards
Oliver
Alan Stern
maintainerQuoted 18 lines · Bryan Wu
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Quoted 2 lines
What is supposed to make all these changes a good idea?
Since buf might not be 4-byte aligned.
It is. Please analyze the code before you use these access methods.
You are right. buf has been 4-byte aligned since 2.6.19. My patch was written two years ago. Sorry for the noise I caused.
I has keeping this patch for a long time. Jie fixed this patch at 2006/09/20 in our svn:
http://blackfin.uclinux.org/git/?p=readonly-mirrors/linux-kernel.git;a=commit;h=cb3da1243f84b37b53486c7e86da34565b4c5d92
http://blackfin.uclinux.org/git/?p=readonly-mirrors/linux-kernel.git;a=commit;h=2559298f0dca2cffc0b87390b92a484004f0d85eAnd a similar patch from David Miller was accept in
ohci-hub.c
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=92164c5dd1ade33f4e90b72e407910de6694de49
Quoted 7 lines
Also because of the same issue, which was fixed by:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=54bee6e1b455573658972510a76119f279db32b7If other functions not only
rh_call_control()call thishub_control()pointer and the buf is not 4-byte aligned,
this bug will fire again without the unaligned API. This patch is safer for the caller, although not efficient.
But the hub_control routines are almost never called by anything other than rh_call_control(). The only exception is in ehci_hub.c, and there the buf pointer is NULL.
So I don't see any need for this patch.
On the other hand, Jie could make a bunch of useful changes to drivers/usb/gadget/file_storage.c, which has its own private routines for unaligned little-endian access.
Alan Stern
PATCH 1/1 usb: file-storage.c use unaligned access helpers
Replace the put_be16/32 and get_be16/32 helpers with the common unaligned access routines. Note that these put_ helpers had the pointer/value parameter in the opposite order from the common version.
--- drivers/usb/gadget/file_storage.c | 79 ++++++++++++------------------------- 1 files changed, 25 insertions(+), 54 deletions(-)
drivers/usb/gadget/file_storage.c +25 -54
@@ -242,6 +242,8 @@#include <linux/usb/ch9.h> #include <linux/usb/gadget.h> +#include <asm/unaligned.h> +#include "gadget_chips.h"
@@ -763,37 +765,6 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct usb_ep *ep)return usb_ep_set_halt (ep); } - -/*-------------------------------------------------------------------------*/ - -/* Routines for unaligned data access */ - -static u16 get_be16 (u8 *buf) -{ -return ((u16 ) buf[0 ] <<8 ) | ((u16 ) buf[1 ]); -} - -static u32 get_be32 (u8 *buf) -{ -return ((u32 ) buf[0 ] <<24 ) | ((u32 ) buf[1 ] <<16 ) | - ((u32 ) buf[2 ] <<8 ) | ((u32 ) buf[3 ]); -} - -static void put_be16 (u8 *buf,u16 val) -{ - buf[0 ] = val >>8 ; - buf[1 ] = val; -} - -static void put_be32 (u8 *buf,u32 val) -{ - buf[0 ] = val >>24 ; - buf[1 ] = val >>16 ; - buf[2 ] = val >>8 ; - buf[3 ] = val &0xff ; -} - -/*-------------------------------------------------------------------------*/ /*
@@ -1551,9 +1522,9 @@ static int do_read(struct fsg_dev *fsg)/* Get the starting Logical Block Address and check that it's * not too big */ if (fsg->cmnd[0 ] ==SC_READ_6 ) - lba = (fsg->cmnd[1 ] <<16 ) |get_be16 (&fsg->cmnd[2 ]); + lba =get_unaligned_be32 (fsg->cmnd) &0x00ffffff ;else { - lba =get_be32 (&fsg->cmnd[2 ]); + lba =get_unaligned_be32 (&fsg->cmnd[2 ]);/* We allow DPO (Disable Page Out = don't save data in the * cache) and FUA (Force Unit Access = don't read from the
@@ -1684,9 +1655,9 @@ static int do_write(struct fsg_dev *fsg)/* Get the starting Logical Block Address and check that it's * not too big */ if (fsg->cmnd[0 ] ==SC_WRITE_6 ) - lba = (fsg->cmnd[1 ] <<16 ) |get_be16 (&fsg->cmnd[2 ]); + lba =get_unaligned_be32 (fsg->cmnd) &0x00ffffff ;else { - lba =get_be32 (&fsg->cmnd[2 ]); + lba =get_unaligned_be32 (&fsg->cmnd[2 ]);/* We allow DPO (Disable Page Out = don't save data in the * cache) and FUA (Force Unit Access = write directly to the
@@ -1920,7 +1891,7 @@ static int do_verify(struct fsg_dev *fsg)/* Get the starting Logical Block Address and check that it's * not too big */ - lba =get_be32 (&fsg->cmnd[2 ]); + lba =get_unaligned_be32 (&fsg->cmnd[2 ]);if (lba >= curlun->num_sectors) { curlun->sense_data =SS_LOGICAL_BLOCK_ADDRESS_OUT_OF_RANGE ;return -EINVAL ;
@@ -1933,7 +1904,7 @@ static int do_verify(struct fsg_dev *fsg)return -EINVAL ; } - verification_length =get_be16 (&fsg->cmnd[7 ]); + verification_length =get_unaligned_be16 (&fsg->cmnd[7 ]);if (unlikely (verification_length ==0 ))return -EIO ;// No default reply
@@ -2078,7 +2049,7 @@ static int do_request_sense(struct fsg_dev *fsg, struct fsg_buffhd *bh)memset (buf,0 ,18 ); buf[0 ] = valid |0x70 ;// Valid, current error buf[2 ] =SK (sd); -put_be32 (&buf[3 ], sdinfo);// Sense information +put_unaligned_be32 (sdinfo, &buf[3 ]);// Sense information buf[7 ] =18 -8 ;// Additional sense length buf[12 ] =ASC (sd); buf[13 ] =ASCQ (sd);
@@ -2089,7 +2060,7 @@ static int do_request_sense(struct fsg_dev *fsg, struct fsg_buffhd *bh)static int do_read_capacity (struct fsg_dev *fsg,struct fsg_buffhd *bh) {struct lun *curlun = fsg->curlun; -u32 lba =get_be32 (&fsg->cmnd[2 ]); +u32 lba =get_unaligned_be32 (&fsg->cmnd[2 ]);int pmi = fsg->cmnd[8 ];u8 *buf = (u8 *) bh->buf;
@@ -2099,8 +2070,8 @@ static int do_read_capacity(struct fsg_dev *fsg, struct fsg_buffhd *bh)return -EINVAL ; } -put_be32 (&buf[0 ], curlun->num_sectors -1 );// Max logical block -put_be32 (&buf[4 ],512 );// Block length +put_unaligned_be32 (curlun->num_sectors -1 , &buf[0 ]);// Max logical block +put_unaligned_be32 (512 , &buf[4 ]);// Block length return 8 ; }
@@ -2158,10 +2129,10 @@ static int do_mode_sense(struct fsg_dev *fsg, struct fsg_buffhd *bh) buf[2 ] =0x04 ;// Write cache enable, // Read cache not disabled // No cache retention priorities -put_be16 (&buf[4 ],0xffff );// Don't disable prefetch +put_unaligned_be16 (0xffff , &buf[4 ]);// Don't disable prefetch // Minimum prefetch = 0 -put_be16 (&buf[8 ],0xffff );// Maximum prefetch -put_be16 (&buf[10 ],0xffff );// Maximum prefetch ceiling +put_unaligned_be16 (0xffff , &buf[8 ]);// Maximum prefetch +put_unaligned_be16 (0xffff , &buf[10 ]);// Maximum prefetch ceiling } buf +=12 ; }
@@ -2178,7 +2149,7 @@ static int do_mode_sense(struct fsg_dev *fsg, struct fsg_buffhd *bh)if (mscmnd ==SC_MODE_SENSE_6 ) buf0[0 ] = len -1 ;else -put_be16 (buf0, len -2 ); +put_unaligned_be16 (len -2 , buf0);return len; }
@@ -2266,8 +2237,8 @@ static int do_read_format_capacities(struct fsg_dev *fsg, buf[3 ] =8 ;// Only the Current/Maximum Capacity Descriptor buf +=4 ; -put_be32 (&buf[0 ], curlun->num_sectors);// Number of blocks -put_be32 (&buf[4 ],512 );// Block length +put_unaligned_be32 (curlun->num_sectors, &buf[0 ]);// Number of blocks +put_unaligned_be32 (512 , &buf[4 ]);// Block length buf[4 ] =0x02 ;// Current capacity return 12 ; }
@@ -2775,7 +2746,7 @@ static int do_scsi_command(struct fsg_dev *fsg)break ;case SC_MODE_SELECT_10 : - fsg->data_size_from_cmnd =get_be16 (&fsg->cmnd[7 ]); + fsg->data_size_from_cmnd =get_unaligned_be16 (&fsg->cmnd[7 ]);if ((reply =check_command (fsg,10 ,DATA_DIR_FROM_HOST , (1 <<1 ) | (3 <<7 ),0 ,"MODE SELECT(10)" )) ==0 )
@@ -2791,7 +2762,7 @@ static int do_scsi_command(struct fsg_dev *fsg)break ;case SC_MODE_SENSE_10 : - fsg->data_size_from_cmnd =get_be16 (&fsg->cmnd[7 ]); + fsg->data_size_from_cmnd =get_unaligned_be16 (&fsg->cmnd[7 ]);if ((reply =check_command (fsg,10 ,DATA_DIR_TO_HOST , (1 <<1 ) | (1 <<2 ) | (3 <<7 ),0 ,"MODE SENSE(10)" )) ==0 )
@@ -2816,7 +2787,7 @@ static int do_scsi_command(struct fsg_dev *fsg)break ;case SC_READ_10 : - fsg->data_size_from_cmnd =get_be16 (&fsg->cmnd[7 ]) <<9 ; + fsg->data_size_from_cmnd =get_unaligned_be16 (&fsg->cmnd[7 ]) <<9 ;if ((reply =check_command (fsg,10 ,DATA_DIR_TO_HOST , (1 <<1 ) | (0xf <<2 ) | (3 <<7 ),1 ,"READ(10)" )) ==0 )
@@ -2824,7 +2795,7 @@ static int do_scsi_command(struct fsg_dev *fsg)break ;case SC_READ_12 : - fsg->data_size_from_cmnd =get_be32 (&fsg->cmnd[6 ]) <<9 ; + fsg->data_size_from_cmnd =get_unaligned_be32 (&fsg->cmnd[6 ]) <<9 ;if ((reply =check_command (fsg,12 ,DATA_DIR_TO_HOST , (1 <<1 ) | (0xf <<2 ) | (0xf <<6 ),1 ,"READ(12)" )) ==0 )
@@ -2840,7 +2811,7 @@ static int do_scsi_command(struct fsg_dev *fsg)break ;case SC_READ_FORMAT_CAPACITIES : - fsg->data_size_from_cmnd =get_be16 (&fsg->cmnd[7 ]); + fsg->data_size_from_cmnd =get_unaligned_be16 (&fsg->cmnd[7 ]);if ((reply =check_command (fsg,10 ,DATA_DIR_TO_HOST , (3 <<7 ),1 ,"READ FORMAT CAPACITIES" )) ==0 )
@@ -2898,7 +2869,7 @@ static int do_scsi_command(struct fsg_dev *fsg)break ;case SC_WRITE_10 : - fsg->data_size_from_cmnd =get_be16 (&fsg->cmnd[7 ]) <<9 ; + fsg->data_size_from_cmnd =get_unaligned_be16 (&fsg->cmnd[7 ]) <<9 ;if ((reply =check_command (fsg,10 ,DATA_DIR_FROM_HOST , (1 <<1 ) | (0xf <<2 ) | (3 <<7 ),1 ,"WRITE(10)" )) ==0 )
@@ -2906,7 +2877,7 @@ static int do_scsi_command(struct fsg_dev *fsg)break ;case SC_WRITE_12 : - fsg->data_size_from_cmnd =get_be32 (&fsg->cmnd[6 ]) <<9 ; + fsg->data_size_from_cmnd =get_unaligned_be32 (&fsg->cmnd[6 ]) <<9 ;if ((reply =check_command (fsg,12 ,DATA_DIR_FROM_HOST , (1 <<1 ) | (0xf <<2 ) | (0xf <<6 ),1 ,"WRITE(12)" )) ==0 )
Signature · 1 line
1.5.5.1.482.g0f174
Alan Stern
maintainerRe: [PATCH] usb: file-storage.c use unaligned access helpers
Quoted 4 lines · Harvey Harrison
Replace the put_be16/32 and get_be16/32 helpers with the common unaligned access routines. Note that these
put_helpers had the pointer/value parameter in the opposite order from the common version.
This is fine and it's what I requested. Still, it's not really the best solution.
In many of the places where these helpers are used, we _know_ that the buffers are in fact aligned properly. For example, in code like this:
Review
Quoted 2 lines
(quoted diff) +1
-1- put_be16(&buf[4], 0xffff); + put_unaligned_be16(0xffff, &buf[4]);
we know that buf is aligned. It would be great if there were helpers which would allow us to do
put_be16(&buf[4], 0xffff);
instead of
* (__be16 *) &buf[4] = cpu_to_be16(0xffff);
They could also be used to clean up the disputed HCD code. Is this a reasonable thing to ask for? A few simple, arch-independent macros would be sufficient.
Alan Stern
[RFC-PATCH] lib: add byteorder helpers for the aligned case
Some users know the pointer they are writing to are aligned, rather than doing *(__le16 *)ptr = cpu_to_le16(val) add helpers wrapping this up that have the same convention as put_unaligned_le/be.
---
Alan, as requested, I'm looking around a bit to see if there are actual users for this. But it does make a nice complement to the unaligned versions.
include/linux/byteorder/generic.h | 30 ++++++++++++++++++++++++++++++ 1 files changed, 30 insertions(+), 0 deletions(-)
include/linux/byteorder/generic.h +30 -0
@@ -119,6 +119,36 @@#define cpu_to_be16s __cpu_to_be16s#define be16_to_cpus __be16_to_cpus +static inline void put_le16 (u16 val,void *ptr) +{ + *(__le16 *)ptr =cpu_to_le16 (val); +} + +static inline void put_le32 (u32 val,void *ptr) +{ + *(__le32 *)ptr =cpu_to_le32 (val); +} + +static inline void put_le64 (u64 val,void *ptr) +{ + *(__le64 *)ptr =cpu_to_le64 (val); +} + +static inline void put_be16 (u16 val,void *ptr) +{ + *(__be16 *)ptr =cpu_to_be16 (val); +} + +static inline void put_be32 (u32 val,void *ptr) +{ + *(__be32 *)ptr =cpu_to_be32 (val); +} + +static inline void put_be64 (u64 val,void *ptr) +{ + *(__be64 *)ptr =cpu_to_be64 (val); +} +/* * They have to be macros in order to do the constant folding * correctly - if the argument passed into a inline function
Signature · 1 line
1.5.5.1.570.g26b5e
Alan Stern
maintainerRe: [RFC-PATCH] lib: add byteorder helpers for the aligned case
Quoted 9 lines · Harvey Harrison
Some users know the pointer they are writing to are aligned, rather than doing *(__le16 *)ptr =
cpu_to_le16(val)add helpers wrapping this up that have the same convention as put_unaligned_le/be.Signed-off-by Harvey Harrison <[email protected]>---
Alan, as requested, I'm looking around a bit to see if there are actual users for this. But it does make a nice complement to the unaligned versions.
This is great -- thanks! I've wanted this sort of thing for a long time. There's a good chance that the SCSI core could make use of it. Try looking at the prep functions in sd.c and sr.c.
include/linux/byteorder/generic.h line 125
Quoted 4 lines
include/linux/byteorder/generic.h +4
-0+static inline void put_le16 (u16 val,void *ptr) +{ + *(__le16 *)ptr =cpu_to_le16 (val); +}
Is this able to do the byte rearrangement at compile time if val is a compile-time constant? I imagine it would.
Alan Stern
Re: [RFC-PATCH] lib: add byteorder helpers for the aligned case
Quoted 10 lines · Alan Stern
include/linux/byteorder/generic.h line 125
Quoted 4 lines · Harvey Harrison
include/linux/byteorder/generic.h +4
-0+static inline void put_le16 (u16 val,void *ptr) +{ + *(__le16 *)ptr =cpu_to_le16 (val); +}Is this able to do the byte rearrangement at compile time if val is a compile-time constant? I imagine it would.
Yes, the cpu_to_le/be functions to get optimized if they are passed a compile time constant.
Harvey
Quoted 1 line · Bryan Wu
Cc Harvey Harrison <[email protected]>
Looks fine.
Quoted 53 lines
Signed-off-by Jie Zhang <[email protected]>Signed-off-by Bryan Wu <[email protected]>--- drivers/usb/host/isp116x-hcd.c | 4 ++-- drivers/usb/host/uhci-hub.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-)drivers/usb/host/isp116x-hcd.c +2
-2@@ -1024,7 +1024,7 @@ static int isp116x_hub_control(struct usb_hcd *hcd,break ;case GetHubStatus:DBG ("GetHubStatus\n" ); - *(__le32 *) buf =0 ; +put_unaligned_le32 (0 , buf);break ;case GetPortStatus:DBG ("GetPortStatus\n" );@@ -1033,7 +1033,7 @@ static int isp116x_hub_control(struct usb_hcd *hcd,spin_lock_irqsave (&isp116x->lock, flags); tmp =isp116x_read_reg32 (isp116x, (--wIndex) ?HCRHPORT2 :HCRHPORT1 );spin_unlock_irqrestore (&isp116x->lock, flags); - *(__le32 *) buf =cpu_to_le32 (tmp); +put_unaligned_le32 (tmp, buf);DBG ("GetPortStatus: port[%d] %08x\n" , wIndex +1 , tmp);break ;case ClearPortFeature:drivers/usb/host/uhci-hub.c +3
-3@@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,switch (typeReq) {case GetHubStatus: - *(__le32 *)buf =cpu_to_le32 (0 ); +put_unaligned_le32 (0 , buf);OK (4 );/* hub power */ case GetPortStatus:if (port >= uhci->rh_numports)@@ -306,8 +306,8 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,dev_dbg (uhci_dev (uhci),"port %d portsc %04x,%02x\n" , wIndex, status, lstatus); - *(__le16 *)buf =cpu_to_le16 (wPortStatus); - *(__le16 *)(buf +2 ) =cpu_to_le16 (wPortChange); +put_unaligned_le16 (wPortStatus, buf); +put_unaligned_le16 (wPortChange, buf +2 );OK (4 );case SetHubFeature:/* We don't implement these */ case ClearHubFeature:
Quoted 1 line · On Thu, 15 May 2008 14:19:24 +0800 Bryan Wu <[email protected]> wrote:
Subject: [PATCH 1/1] [usb/host]: use get/put_unaligned_* helpers to fix more potential unaligned issues.
Please don't put the subsystem identifier ("usb/host") inside square brackets. Because text in square brackets is considered "not part of the patch title" and gets thrown away. It contains text such as "patch", "BUG", "RFC", "2.6.26-rc2", etc.
That should be in Documentation/SubmittingPatches but I can't find it. hrm.