netdev - Re: [PATCH] SMSC LAN911x and LAN921x vendor driver

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 linux-hardening linux-cve-announce 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]

Message-ID: <20080602190329.GC6192@solarflare.com>
Date:	Mon, 2 Jun 2008 20:03:30 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Steve.Glendinning@...c.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

Steve.Glendinning@...c.com wrote:
> 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?

If you can't reserve some range of values for errors (e.g. 32-bit reads
which may yield any 32-bit value) then I suppose so.  For 16-bit reads you
can return an int with negative values reserved for errors.

> 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.

Hardware failure does happen though, and once you've detected it it seems
like a bad idea to hide it.

> > [...]
> > > +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?

Personally I think -EIO is a perfectly reasonable error code for I/O
errors after the device has been positively identified, even during
probe.  You could have the probe function squash other errors into
-ENODEV if you want.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
--
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