All of lore.kernel.org
 help / color / mirror / Atom feed
From: Byungho An <bh74.an@samsung.com>
To: 'David Miller' <davem@davemloft.net>, dan.carpenter@oracle.com
Cc: ks.giri@samsung.com, siva.kallam@samsung.com,
	vipul.pandya@samsung.com, netdev@vger.kernel.org,
	kernel-janitors@vger.kernel.org
Subject: RE: [patch 1/3] net: sxgbe: sxgbe_mdio_register() frees the bus
Date: Wed, 02 Apr 2014 06:55:51 +0000	[thread overview]
Message-ID: <000301cf4e40$97f14a40$c7d3dec0$@samsung.com> (raw)
In-Reply-To: <20140401.162322.2243641448457002118.davem@davemloft.net>

David Miller <davem@davemloft.net> :
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Date: Tue, 1 Apr 2014 16:38:44 +0300
> 
> > @@ -219,13 +219,6 @@ int sxgbe_mdio_register(struct net_device *ndev)
> >  		}
> >  	}
> >
> > -	if (!err) {
> > -		netdev_err(ndev, "PHY not found\n");
> > -		mdiobus_unregister(mdio_bus);
> > -		mdiobus_free(mdio_bus);
> > -		goto mdiobus_err;
> > -	}
> > -
> ...
> > @@ -93,9 +93,9 @@ static void sxgbe_core_set_umac_addr(void __iomem
> > *ioaddr, unsigned char *addr,  {
> >  	u32 high_word, low_word;
> >
> > -	high_word = (addr[5] << 8) || (addr[4]);
> > -	low_word = ((addr[3] << 24) || (addr[2] << 16) ||
> > -		    (addr[1] << 8) || (addr[0]));
> > +	high_word = (addr[5] << 8) | (addr[4]);
> > +	low_word = (addr[3] << 24) | (addr[2] << 16) |
> > +		   (addr[1] << 8) | (addr[0]);
> >  	writel(high_word, ioaddr + SXGBE_CORE_ADD_HIGHOFFSET(reg_n));
> >  	writel(low_word, ioaddr + SXGBE_CORE_ADD_LOWOFFSET(reg_n));  }
> 
> Nothing says "DRIVER NOT TESTED" like these two bugs.
> 
> The MDIO bus is always freed, and the MAC address is corrupted into a
> boolean value before being programmed into the hardware.
> 
> Frankly, this kind of stuff is unacceptable.
> 
> I cannot see how this driver can function successfully with these two
errors, it
> looks simply impossible.
> 
> I'm not going to hide my feelings, this was a truly terrible driver
submission.
> The amount of reviewing resources consumed during all of these iterations
was
> huge, and it still went in with bugs like this.
> 
> It probably should have gone into staging.

Oops, my apologies. mdio err path and logical and bitwise for mac address are
my mistake I didn't tested carefully (maybe tested with previous version)
because I couldn't much time to test it due to facing merge window. sorry
about that...

For logical and bitwise OR for mac address, actually I couldn't catch since
"eth_hw_addr_random" is used.. however it is also mistake.

I apology again.



WARNING: multiple messages have this Message-ID (diff)
From: Byungho An <bh74.an@samsung.com>
To: 'David Miller' <davem@davemloft.net>, dan.carpenter@oracle.com
Cc: ks.giri@samsung.com, siva.kallam@samsung.com,
	vipul.pandya@samsung.com, netdev@vger.kernel.org,
	kernel-janitors@vger.kernel.org
Subject: RE: [patch 1/3] net: sxgbe: sxgbe_mdio_register() frees the bus
Date: Tue, 01 Apr 2014 23:55:51 -0700	[thread overview]
Message-ID: <000301cf4e40$97f14a40$c7d3dec0$@samsung.com> (raw)
In-Reply-To: <20140401.162322.2243641448457002118.davem@davemloft.net>

David Miller <davem@davemloft.net> :
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Date: Tue, 1 Apr 2014 16:38:44 +0300
> 
> > @@ -219,13 +219,6 @@ int sxgbe_mdio_register(struct net_device *ndev)
> >  		}
> >  	}
> >
> > -	if (!err) {
> > -		netdev_err(ndev, "PHY not found\n");
> > -		mdiobus_unregister(mdio_bus);
> > -		mdiobus_free(mdio_bus);
> > -		goto mdiobus_err;
> > -	}
> > -
> ...
> > @@ -93,9 +93,9 @@ static void sxgbe_core_set_umac_addr(void __iomem
> > *ioaddr, unsigned char *addr,  {
> >  	u32 high_word, low_word;
> >
> > -	high_word = (addr[5] << 8) || (addr[4]);
> > -	low_word = ((addr[3] << 24) || (addr[2] << 16) ||
> > -		    (addr[1] << 8) || (addr[0]));
> > +	high_word = (addr[5] << 8) | (addr[4]);
> > +	low_word = (addr[3] << 24) | (addr[2] << 16) |
> > +		   (addr[1] << 8) | (addr[0]);
> >  	writel(high_word, ioaddr + SXGBE_CORE_ADD_HIGHOFFSET(reg_n));
> >  	writel(low_word, ioaddr + SXGBE_CORE_ADD_LOWOFFSET(reg_n));  }
> 
> Nothing says "DRIVER NOT TESTED" like these two bugs.
> 
> The MDIO bus is always freed, and the MAC address is corrupted into a
> boolean value before being programmed into the hardware.
> 
> Frankly, this kind of stuff is unacceptable.
> 
> I cannot see how this driver can function successfully with these two
errors, it
> looks simply impossible.
> 
> I'm not going to hide my feelings, this was a truly terrible driver
submission.
> The amount of reviewing resources consumed during all of these iterations
was
> huge, and it still went in with bugs like this.
> 
> It probably should have gone into staging.

Oops, my apologies. mdio err path and logical and bitwise for mac address are
my mistake I didn't tested carefully (maybe tested with previous version)
because I couldn't much time to test it due to facing merge window. sorry
about that...

For logical and bitwise OR for mac address, actually I couldn't catch since
"eth_hw_addr_random" is used.. however it is also mistake.

I apology again.



  reply	other threads:[~2014-04-02  6:55 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-04-01 13:38 [patch 1/3] net: sxgbe: sxgbe_mdio_register() frees the bus Dan Carpenter
2014-04-01 13:38 ` Dan Carpenter
2014-04-01 20:23 ` David Miller
2014-04-01 20:23   ` David Miller
2014-04-02  6:55   ` Byungho An [this message]
2014-04-02  6:55     ` Byungho An
2014-04-01 20:27 ` David Miller
2014-04-01 20:27   ` David Miller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='000301cf4e40$97f14a40$c7d3dec0$@samsung.com' \
    --to=bh74.an@samsung.com \
    --cc=dan.carpenter@oracle.com \
    --cc=davem@davemloft.net \
    --cc=kernel-janitors@vger.kernel.org \
    --cc=ks.giri@samsung.com \
    --cc=netdev@vger.kernel.org \
    --cc=siva.kallam@samsung.com \
    --cc=vipul.pandya@samsung.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.