[PATCH 8/8] staging: rtl8192u: fix read_nic_* functions

Dan Carpenter dan.carpenter at oracle.com
Wed Jun 5 08:06:29 UTC 2013


My main concern here is that in eprom_ck_cycle() the original code
read twice and in the new code it only reads one time.

What you have done is *probably* what we want to do and it's
*probably* an improvement over the original but we can't know
without testing it and I don't think you have the hardware to test
this.  So let's put two reads back in and add a comment.

void eprom_ck_cycle(struct net_device *dev)
{
	u8 cmdreg;
	int ret;

	ret = read_nic_byte_E(dev, EPROM_CMD, &cmdreg);
	if (ret)
		return;
	write_nic_byte_E(dev, EPROM_CMD, cmdreg | (1<<EPROM_CK_SHIFT));
	force_pci_posting(dev);
	udelay(EPROM_DELAY);
	/* I'm not sure this second read is needed but I can't test it */
	ret = read_nic_byte_E(dev, EPROM_CMD, &cmdreg);
	if (ret)
		return;
	write_nic_byte_E(dev, EPROM_CMD, cmdreg & ~(1<<EPROM_CK_SHIFT));
	force_pci_posting(dev);
	udelay(EPROM_DELAY);
}

Also read_nic_byte_E() now returns an error code but we don't check
it so we've just moved the uninitialized code problem around a bit
but not fixed it.  The error handling could be added in a follow on
patch because that doesn't introduce any bugs that weren't there in
the original code.

The other minor style issue is that there should be a blank line
between declarations and code.

int read_nic_byte(struct net_device *dev, int x, u8 *data);
int read_nic_byte_E(struct net_device *dev, int x, u8 *data);
int read_nic_dword(struct net_device *dev, int x, u32 *data);
int read_nic_word(struct net_device *dev, int x, u16 *data) ;

These should be renamed to read_nic_u8(), read_nic_u8_E(),
read_nic_u32() and read_nic_u16().  What does the "_E" mean?  That's
actually a terrible name, obviously.

Also btw, the (1<<EPROM_CK_SHIFT) could be replaced with:
#define EPROM_CK_BIT		BIT(2)

	write_nic_byte_E(dev, EPROM_CMD, cmdreg | EPROM_CK_BIT);
and
	write_nic_byte_E(dev, EPROM_CMD, cmdreg & ~EPROM_CK_BIT);

> In function eprom_r(), defined in r8180_93cx6.c,
> a value with type 'u8' is assigned to a variable of
> type 'short'. I cannot think of a reason to do that,
> given the context, but I left it as it is.
> 

There isn't any reason for it.  Feel free to change it if you want.

> -u8 read_nic_byte(struct net_device *dev, int indx)
> +int read_nic_byte(struct net_device *dev, int indx, u8 *data)
>  {
> -	u8 data;
>  	int status;
>  	struct r8192_priv *priv = (struct r8192_priv *)ieee80211_priv(dev);
>  	struct usb_device *udev = priv->udev;
>  
>  	status = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>  				 RTL8187_REQ_GET_REGS, RTL8187_REQT_READ,
> -				 (indx&0xff)|0xff00, (indx>>8)&0x0f, &data, 1, HZ / 2);
				 (indx & 0xff) | 0xff00, (indx >> 8) & 0x0f,
				 data, 1, HZ / 2);

Also "indx" is a weird way to abreviate.  Normally it would be "idx"
or spelled out all the way "index".

>  #ifdef DEBUG_RX
> -	DMESG("rxconf: %x %x", rxconf, read_nic_dword(dev, RCR));
> +	{
> +		u32 tmp;
> +		read_nic_dword(dev, RCR, &tmp);
> +		DMESG("rxconf: %x %x", rxconf, tmp);
> +	}
>  #endif

Delete!

regards,
dan carpenter




More information about the devel mailing list