Re: [PATCH] staging: octeon: fixed few coding style warnings

[David Daney <ddaney@caviumnetworks.com>]

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