Re: net-next: KSZ switch driver oops in ksz_mib_read_work

[Andrew Lunn <andrew@lunn.ch>]

On Tue, Jun 11, 2019 at 04:27:47PM -0700, Florian Fainelli wrote:
> On 6/11/19 10:57 AM, Robert Hancock wrote:
> > We are using an embedded platform with a KSZ9897 switch. I am getting
> > the oops below in ksz_mib_read_work when testing with net-next branch.
> > After adding in some debug output, the problem is in this code:
> > 
> > 	for (i = 0; i < dev->mib_port_cnt; i++) {
> > 		p = &dev->ports[i];
> > 		mib = &p->mib;
> > 		mutex_lock(&mib->cnt_mutex);
> > 
> > 		/* Only read MIB counters when the port is told to do.
> > 		 * If not, read only dropped counters when link is not up.
> > 		 */
> > 		if (!p->read) {
> > 			const struct dsa_port *dp = dsa_to_port(dev->ds, i);
> > 
> > 			if (!netif_carrier_ok(dp->slave))
> > 				mib->cnt_ptr = dev->reg_mib_cnt;
> > 		}
> > 
> > The oops is happening on port index 3 (i.e. 4th port) which is not
> > connected on our platform and so has no entry in the device tree. For
> > that port, dp->slave is NULL and so netif_carrier_ok explodes.
> > 
> > If I change the code to skip the port entirely in the loop if dp->slave
> > is NULL it seems to fix the crash, but I'm not that familiar with this
> > code. Can someone confirm whether that is the proper fix?
> 
> Yes, the following should do it, if you confirm that is the case, I can
> send that later with your Tested-by.
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c
> b/drivers/net/dsa/microchip/ksz_common.c
> index 39dace8e3512..5470b28332cf 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -93,6 +93,9 @@ static void ksz_mib_read_work(struct work_struct *work)
>                 if (!p->read) {
>                         const struct dsa_port *dp = dsa_to_port(dev->ds, i);
> 
> +                       if (dsa_is_unused_port(dp))
> +                               continue;
> +
>                         if (!netif_carrier_ok(dp->slave))
>                                 mib->cnt_ptr = dev->reg_mib_cnt;
>                 }
> 

Hi Florian

There is a mutex held within the loop. So a continue is not going to
work here.

     Andrew