| 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
| ||
|
Date: Wed, 14 Oct 2015 09:09:04 -0700
From: David Daney <ddaney@...iumnetworks.com>
To: Sakshi Bansal <sakshi.april5@...il.com>
CC: <aaro.koskinen@...ia.com>, <joe@...ches.com>,
<janne.huttunen@...ia.com>, <arnd@...db.de>,
<linux-kernel@...r.kernel.org>, <jamie.lawler@...il.com>,
<gregkh@...uxfoundation.org>,
Paul Martin <paul.martin@...ethink.co.uk>,
<david.daney@...ium.com>, <ralf@...ux-mips.org>,
<okash.khawaja@...il.com>, <robertoxmed@...il.com>,
<aybuke.147@...il.com>, <helen.fornazier@...il.com>,
<jtk54@...nell.edu>, <devel@...verdev.osuosl.org>
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@...iumnetworks.com
> <mailto:ddaney@...iumnetworks.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@...iumnetworks.com <mailto:ddaney@...iumnetworks.com>
> <mailto:ddaney@...iumnetworks.com
> <mailto:ddaney@...iumnetworks.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@...il.com
> <mailto:sakshi.april5@...il.com>
> <mailto:sakshi.april5@...il.com
> <mailto:sakshi.april5@...il.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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists