lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 2 Jun 2008 19:30:59 +0100
From:	Steve.Glendinning@...c.com
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	Bahadir Balban <Bahadir.Balban@....com>,
	Bill Gatliff <bgat@...lgatliff.com>, catalin.marinas@....com,
	Dustin Mcintire <dustin@...soria.com>, Enrik.Berkhan@...com,
	hennerich@...ckfin.uclinux.org, ian.saturley@...c.com,
	Michael.Hennerich@...log.com, netdev@...r.kernel.org,
	uclinux-dist-devel@...ckfin.uclinux.org
Subject: Re: [PATCH] SMSC LAN911x and LAN921x vendor driver

Hi Ben,

Thanks for the feedback.  I've implemented most of your suggestions, but 
have a few questions:

> [...]
> > +/* Set a mac register, phy_lock must be acquired before calling */
> > +static void smsc911x_mac_write(struct smsc911x_data *pdata,
> > +                unsigned int offset, u32 val)
> > +{
> > +   unsigned int temp;
> > +
> > +#ifdef CONFIG_DEBUG_SPINLOCK
> > +   if (!spin_is_locked(&pdata->phy_lock))
> > +      SMSC_WARNING("phy_lock not held");
> > +#endif            /* CONFIG_DEBUG_SPINLOCK */
> > +
> > +   temp = smsc911x_reg_read(pdata, MAC_CSR_CMD);
> > +   if (unlikely(temp & MAC_CSR_CMD_CSR_BUSY_)) {
> > +      SMSC_WARNING("smsc911x_mac_write failed, MAC busy at entry");
> > +      return;
> > +   }
> 
> Shouldn't this return an error code?
> 
> [...]
> > +/* Sets a phy register, phy_lock must be acquired before calling */
> > +static void smsc911x_phy_write(struct smsc911x_data *pdata,
> > +                unsigned int index, u16 val)
> > +{
> > +   unsigned int addr;
> > +   int i;
> > +
> > +#ifdef CONFIG_DEBUG_SPINLOCK
> > +   if (!spin_is_locked(&pdata->phy_lock))
> > +      SMSC_WARNING("phy_lock not held");
> > +#endif            /* CONFIG_DEBUG_SPINLOCK */
> > +
> > +   /* Confirm MII not busy */
> > +   if (unlikely(smsc911x_mac_read(pdata, MII_ACC) & 
MII_ACC_MII_BUSY_)) {
> > +      SMSC_WARNING("MII is busy in smsc911x_write_phy???");
> > +      return;
> > +   }
> 
> Similarly for this function.

I could return an error code from these write functions, but the 
equivalent read functions currently return their register value.  Would 
you change the read functions to take a result pointer?

I should mention this "MAC busy at entry" check is an assert to catch 
driver locking problems during development (as is the spinlock check 
above).  If the driver is "correct" it should only be seen in the case of 
hardware failure.

> [...]
> > +static int smsc911x_soft_reset(struct smsc911x_data *pdata)
> > +{
> > +   unsigned int timeout;
> > +   unsigned int temp;
> > +
> > +   /* Reset the LAN911x */
> > +   smsc911x_reg_write(HW_CFG_SRST_, pdata, HW_CFG);
> > +   timeout = 10;
> > +   do {
> > +      udelay(10);
> > +      temp = smsc911x_reg_read(pdata, HW_CFG);
> > +   } while ((--timeout) && (temp & HW_CFG_SRST_));
> > +
> > +   if (unlikely(temp & HW_CFG_SRST_)) {
> > +      SMSC_WARNING("Failed to complete reset");
> > +      return -ENODEV;
> 
> I think this should be -EIO unless this is only called during probe.

This reset function is called from both probe and open, although its 
return code is only checked for nonzero by both.  Should probe return 
-ENODEV, and open return -EIO if this device reset fails?

> [...]
> > +/* SMSC911x registers and bitfields */
> > +#define RX_DATA_FIFO         0x00
> > +
> > +#define TX_DATA_FIFO         0x20
> > +#define TX_CMD_A_ON_COMP_      0x80000000
> 
> Why do these flag/mask names have trailing underscores?

"It was like that when i got here" :-)  The register offsets don't, but 
the bitmasks do.  Should I remove them?

Regards,
--
Steve Glendinning
SMSC GmbH
m: +44 777 933 9124
e: steve.glendinning@...c.com

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists