Messages in this thread | | | Date | Wed, 14 Oct 2015 09:09:04 -0700 | From | David Daney <> | Subject | Re: [PATCH] staging: octeon: fixed few coding style warnings |
| |
On 10/14/2015 09:04 AM, Sakshi Bansal wrote: > Well I did not test the patch
Never send patches to code that you don't at lease attempt to compile.
> since the changes were mostly coding style > related or would not affect the code. > > As for the null check, if the original check is valid I can remove them > from the patch.
They are valid, and I have seen no good reason presented to change them.
If you want to churn up the code with white space changes, I cannot prevent it, but at least accurately describe what you are doing, and make sure you don't break the driver.
David Daney
> > On Oct 14, 2015 9:27 PM, "David Daney" <ddaney@caviumnetworks.com > <mailto:ddaney@caviumnetworks.com>> wrote: > > On 10/14/2015 08:51 AM, Sakshi Bansal wrote: > > I am testing it on Fedora 21. > > > Does your Fedora 21 platform use this driver in any way? Does it > even build the driver (produce any .o files for any of the files you > changed)? > > No, I didn't think so. > > You have to have a way to test the patch (or at least compile the > files), or have other people test it for you if you make changes. > > > The change under concern was mentioned as > a "Check ". > > > Yes, notice that 'CHECK:' is emitted in the color green. It is not > a warning. Your patch subject line said you were fixing warnings. > This is not true. > > > > On Oct 14, 2015 9:17 PM, "David Daney" > <ddaney@caviumnetworks.com <mailto:ddaney@caviumnetworks.com> > <mailto:ddaney@caviumnetworks.com > <mailto:ddaney@caviumnetworks.com>>> wrote: > > On 10/14/2015 07:06 AM, Sakshi Bansal wrote: > > Fixed allignment issues and line over 80 characters > > > Use spell checking on 'allignment' > > But that is not the main problem with the patch... > > > You are changing things other than white space and comment > formatting, can you tell us on which platforms the patch > was tested > to verify that you didn't break anything? > > > Signed-off-by: Sakshi Bansal <sakshi.april5@gmail.com > <mailto:sakshi.april5@gmail.com> > <mailto:sakshi.april5@gmail.com > <mailto:sakshi.april5@gmail.com>>> > > > > NAK. > > [...] > > diff --git a/drivers/staging/octeon/ethernet-mdio.c > b/drivers/staging/octeon/ethernet-mdio.c > index fd9b3d8..590a6cb 100644 > --- a/drivers/staging/octeon/ethernet-mdio.c > +++ b/drivers/staging/octeon/ethernet-mdio.c > @@ -180,7 +180,7 @@ int cvm_oct_phy_setup_device(struct > net_device *dev) > priv->phydev = of_phy_connect(dev, phy_node, > cvm_oct_adjust_link, 0, > > PHY_INTERFACE_MODE_GMII); > > - if (priv->phydev == NULL) > + if (!priv->phydev) > > > > Not a coding style change. There is no WARNING generated > for this case. > > > > return -ENODEV; > > priv->last_link = 0; > diff --git a/drivers/staging/octeon/ethernet-mem.c > b/drivers/staging/octeon/ethernet-mem.c > index 5a5cdb3..d6172e4 100644 > --- a/drivers/staging/octeon/ethernet-mem.c > +++ b/drivers/staging/octeon/ethernet-mem.c > @@ -34,7 +34,7 @@ static int cvm_oct_fill_hw_skbuff(int > pool, > int size, int elements) > while (freed) { > struct sk_buff *skb = > dev_alloc_skb(size + 256); > > - if (unlikely(skb == NULL)) > + if (unlikely(!skb)) > > > Same > > break; > skb_reserve(skb, 256 - (((unsigned > long)skb->data) & 0x7f)); > *(struct sk_buff **)(skb->data - > sizeof(void > *)) = skb; > @@ -98,7 +98,7 @@ static int cvm_oct_fill_hw_memory(int > pool, > int size, int elements) > * just before the block. > */ > memory = kmalloc(size + 256, GFP_ATOMIC); > - if (unlikely(memory == NULL)) { > + if (unlikely(!memory)) { > > > Same > > pr_warn("Unable to allocate %u > bytes > for FPA pool %d\n", > elements * size, pool); > break; > diff --git a/drivers/staging/octeon/ethernet-rgmii.c > b/drivers/staging/octeon/ethernet-rgmii.c > index 51dcb61..3d7513c 100644 > --- a/drivers/staging/octeon/ethernet-rgmii.c > +++ b/drivers/staging/octeon/ethernet-rgmii.c > @@ -68,7 +68,7 @@ static void cvm_oct_rgmii_poll(struct > net_device *dev) > struct octeon_ethernet *priv = netdev_priv(dev); > unsigned long flags = 0; > cvmx_helper_link_info_t link_info; > - int use_global_register_lock = (priv->phydev == > NULL); > + int use_global_register_lock = (!priv->phydev); > > > Same. > > I could go on, but I think you see the pattern here. > > Your changelog says you are fixing warnings, but none of > these are > warning fixes. > > In fact it is perfectly acceptable to compare a pointer to > NULL. It > is a common idiom in the kernel. The original author of > the code > thought it was more clear this way, and you are causing > code churn > for no reason. > > Try to run this command on the kernel sources: > $ git grep -e '== NULL' | wc -l > 21488 > > I would suggest that you convince people that the other > 21,000 cases > of comparison to NULL need changing before you do it to > this driver. > > David Daney > >
| |