From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754189AbbJNQJP (ORCPT ); Wed, 14 Oct 2015 12:09:15 -0400 Received: from mail-bn1bon0073.outbound.protection.outlook.com ([157.56.111.73]:61856 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932371AbbJNQJN (ORCPT ); Wed, 14 Oct 2015 12:09:13 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=David.Daney@caviumnetworks.com; Message-ID: <561E7E20.3030609@caviumnetworks.com> Date: Wed, 14 Oct 2015 09:09:04 -0700 From: David Daney User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Sakshi Bansal CC: , , , , , , , Paul Martin , , , , , , , , Subject: Re: [PATCH] staging: octeon: fixed few coding style warnings References: <20151014140637.GA23007@localhost.localdomain> <561E7920.2000903@caviumnetworks.com> <561E7B74.6010304@caviumnetworks.com> In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [64.2.3.194] X-ClientProxiedBy: BLUPR07CA0035.namprd07.prod.outlook.com (10.255.223.148) To DM3PR07MB2140.namprd07.prod.outlook.com (25.164.4.146) X-Microsoft-Exchange-Diagnostics: 1;DM3PR07MB2140;2:+I6HEhsZUOivaoLglX81AoW0+U/v5EIj7bALy0IkTuP6K8cdQd66q/RFj8c0hvPL0Z8hn9gEJC3OhvOn0II/88UN7WQM5b+umC7E+yxnqliCCPyfL5VuM5sdGQUa7Vg1S0lf4QSq8CABt23ciauuJpHeUSaDdhMm2oVywYHLJyI=;3:WpsCBVAmzPTvHtO7tc/ivWy7YUI4jz48t8PzJ/JsxGfpEZfq39mAiPKY+5WdYNznS+Za9Ilc9qX1Kx9q247BNSblbxNHzaOEl/PeLGRbOhkw1BtRZHuWEHYCJ/SUM8In9NGSaeSTJYcTy+YV72nQhQ==;25:l7Ez8mrg0Jm7F1u/ZcSb3SmUlmngM0nifNE/mBqU72e9NtDetOGgo3u7S0x2x6dQiT6JZEFV0qF3+2YSvEBNSU2oseDGKQuVtFM2cmWNF5XNH2YYcVGpwChHAt6aklnz2OC7brp8BoWEhb0Vwi7dodSsgA7VC+3VMG4YjC4uMqmhwEoUT7F5ti/SPuaSoC2OK6h5pAVj05JHHe6AXhT4aDJpbExXCQuOZYWloH4I8Ar5aQY/o8pZ1gaobQXvExjPBBrgeViCg4eLbQdptyjKxw== X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:DM3PR07MB2140; X-Microsoft-Exchange-Diagnostics: 1;DM3PR07MB2140;20:RUWJJ+/C4Zua1z5hCM24FnwB+78+vmBoy/VJmHL3JpEsDZKx58RADaxwnqioBClaRxMOYz6x11tAL3IV06d2YLkxaFHvqK2bMME4YosodXq537HlpZIlqsULGdWXfVmSGQi96aa8mcK1g5qfRWsr78EnyprZ7+Pyh0KIKXEtcYilPh6eV6D6Y8NkvyiAb7PNHPWhHImg43m6I1SyZj8KkxjiTHxNbXKkisArQK3uOms5tT7diT4XTrPDLWSrjGrb3fMUWmjhnDsGo1HeV9dIjsISEtKV/XxOKDbMEfNpN5aCRCxZwTnMu9jQ4gytssPjv2ptpAcPX9eqReCmODqYJXL2RpMTTBdF7sTMEaek9178OVVxmpa+TbUZjb/QTelQrlOjzkMbfbJv3M640RHYINxON+s4cUGhqF2vYTHSyZCQsSPDKAvtm4WCU0eXJJW1JW6ao7hct5YirYz2/W+GQayL+DHG3wInnWsC8pyxZed4Qx1TkHB2jRwUkw1vMx7e6TfdK2tCwJGM/F7hlilF19dO8+W7V/khnV/tFv6GN8hYWBKtP2zOZ38smMFNbYs2nieKGocBMjAnpVC/OmUfgrA+8M01dQG/F4xXP/aYD9Q= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(236414709691187); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(5005006)(520078)(8121501046)(3002001);SRVR:DM3PR07MB2140;BCL:0;PCL:0;RULEID:;SRVR:DM3PR07MB2140; X-Microsoft-Exchange-Diagnostics: 1;DM3PR07MB2140;4:I3M9AX5CXh6UyT+kuCfLVP0eQTtdRT1im0+/HZ9xEjq3h+pS5yYKZq/nVc984JQ993orj3detSVMCKQY/Gw3THBRvBoILVcHoeCf5/TKuEYcC6Dbl7WvlW/OCOx826LDdCFrPEL59NEQzDpGh/qDkSFM1nk0X3n18PK0yrPEEwrM0Gh+y0UbSloXeg1Hk6zdHHuusoYCZBfcgxrjxdMU5jw6vG4rdW/hbZ0yRDXlukK3j1rybDfYSEu14TRkzRC9Ox7kwcZxrwxlNKGieQQBIk5K16dI36AgKT6ZitTW1WsLxTPqXkAJlAlEnj84kfdUhI8bdb8ZTIoiBJdvXgYgUHga+2ohIl5t36PkJtHRP1g= X-Forefront-PRVS: 0729050452 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(6009001)(24454002)(199003)(54534003)(479174004)(189002)(377454003)(46102003)(101416001)(47776003)(33656002)(36756003)(110136002)(87976001)(64706001)(64126003)(50466002)(105586002)(77096005)(23676002)(189998001)(106356001)(5001960100002)(80316001)(59896002)(5008740100001)(5007970100001)(53416004)(93886004)(76176999)(66066001)(4001350100001)(92566002)(19580395003)(65816999)(97736004)(65806001)(19580405001)(54356999)(50986999)(2950100001)(40100003)(5004730100002)(65956001)(69596002)(122386002)(42186005)(87266999)(81156007)(83506001);DIR:OUT;SFP:1101;SCL:1;SRVR:DM3PR07MB2140;H:dl.caveonetworks.com;FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtETTNQUjA3TUIyMTQwOzIzOkJXaGxIdGlxUVp2WE10aDhab2tLVmRiM0Zn?= =?utf-8?B?NDliUndmVFY1OU5SMThJbDdpbTNKd0tscTd1ZnlhNExRdXJKOWp4QzBiSTA1?= =?utf-8?B?OVNJME1jQTR1Lyt5eHZkNE1qOG12SEM2TUtrNkJvR3dXUjgxRG9hekllRnBJ?= =?utf-8?B?RXRscXo4SEdWcVMwNG1Fdnd6QVd4SW55RlBGNVBjb3BWbCtaSzlmU2hadmJN?= =?utf-8?B?YWVDdXZUbTlMaEdtTmhjZjNlWmV3blYwd1RrMERGQUx5YVlzdlQwOXROL2pn?= =?utf-8?B?TDM5eFZ1RzN0ZEhOM2t4Tm1PMHg0SVU3L0JmVVp3WVJYMlNBT2pXVy8vdm9v?= =?utf-8?B?QVdNUk1kQXc1SmV6UGptM0hYTjF6aDNPRUl6OGhEc3JtQXhyWCtyQnZ1U2Fp?= =?utf-8?B?RDQvcXRENUxaS3E5UmdSVERGZmMwblRUZ1FwZlZOTHJBdkVzVkQ2clRhQVhh?= =?utf-8?B?VlJORTJoa3VlbXBtU25FZmsrQ2Z4ZFEyS1prSjFPMEtuSVB0YWpzTU9USW5a?= =?utf-8?B?VXlWM0dOaVdnTlo1dUtWOXdZQ09hdFlVR1VLTllldUN6NWRwV2V4bjdNNE9B?= =?utf-8?B?emlUR3lDNzlNNTlPZkhHc3dLbEZjUWRvVjlzYjlmWWdnSTBBZ3NXalA2NnQ5?= =?utf-8?B?RTZXNElaSXVkQjhhZW9KWXdLbzViR2tiaHJqT1lKaUxxRjRmREhPcnlCeVBI?= =?utf-8?B?MUV6NEl3eUVlNVRNb3YzVTJIWkRjS1M3UmtEcEF6eitrTkFwcm4yaWtoc1Zy?= =?utf-8?B?ejVLbXI4N3JxU0NZOU1tbFF0VzVXV1dxOEQ1eGpzNXRFMG5ldUFDK01XZmZ2?= =?utf-8?B?T096OC9LdVN2K2JnK3QzNFpPVW1XM09icDk5Lyt4aVFLRnkxOXZWY1hDZkxE?= =?utf-8?B?U0w2blErYW41RmtieUZ4a01TQ3lMajBzV014d05PTSt5U1ZXNE9JZGlac05D?= =?utf-8?B?Q0hWYzRmWTNMcGQwZkMyQXlhQkxhbWZiS210N0hMTnQyMytHQk5PcGE5NTNJ?= =?utf-8?B?MjhXeFJFaVczQjc4WjlLdTNBVFZOV2tPVlI3eVpyaWJaZm42TlNrclNGTjlT?= =?utf-8?B?WVdqNzJLaGpTblVwS0tFTm84NVc2TnNjYkFTQU56UXJSa1MzSzZBeG5RVzZj?= =?utf-8?B?aFJicmIvMDRIelB4TVpSOCtpM3JqMG5vZEdidHg2Z0VRVkw5QVRSYVpPL2Zo?= =?utf-8?B?RXFEcG52eW9OVnQ3bGZGeU9zc0ZjMVFqc3ZGbVBPa3dlbkpCK2lBTWZKYWpT?= =?utf-8?B?ZXhRNGE2M1FjMFBZb09tbERPcWJIenYzQmRPZE9hbGoxVGlJOXI2N2ZEeWtP?= =?utf-8?B?c0tjNTJjOGJaMHlDTGQ4ZUxYWklDOFhiS3Jla2oyeUUvWFFJVXMzZFBjUlR3?= =?utf-8?B?blVDQk1JSWJ0ZG9iaWtrN01LUFExdENoNFAxRXR4TFd4dUVkcGhMYURQTS8w?= =?utf-8?B?WjVzcUNkeWRzSzE2aVc3TTRWaUFYMkdybTc5N1RiY3dRUllmVGg5cHdkMWxB?= =?utf-8?B?NWY4aFFMZVl2a0NaZmc0VVQxT3dVT3FwdWRna0JaV3gzVFBJVjUzRSs4RThL?= =?utf-8?B?TWxNbDR1MFh2VjRKK2QrR0JVakVWRmg0RVh5VWZMRWphRGI5cDNHelFJNENO?= =?utf-8?B?cnZLTGNacm56ek5oODlwenY5Rit5U0pPMTY5a3pVcE5tRDJmbkdYUnVqUmVl?= =?utf-8?B?a3R3TVgzdUgvb0FlVmtVbWVzY2puOFE3RUJrQkVmS1doUHRnL1cyVWRpZDhx?= =?utf-8?Q?jPRux2szjEfBhrPfzE248U7biEWqYZXPck1Lk=3D?= X-Microsoft-Exchange-Diagnostics: 1;DM3PR07MB2140;5:FKLrj8Xu61daZ2+N4Z5pkw1jtqj54X3eYZ6x0OsnrSjtUiI/jhZQNCRcYAhj0lX0r1w6SEHYq8jfk/5m/lZ0KsBocX4Giysn5lV3eglICKZjOsihpZVxUZ4E3AsL8ZlCtYOniATF0yenjI99ALdGrQ==;24:iz6R0UmUOjx61jFhQ5O/Ij7f0Y2EijjvA8lAjqBw9KrsWjL1Lq9qof3cV/UBKYY3Tnf3Wm6am5iwXW5yi9yYlV02se5FRfUpdIeooqGNuO8=;20:7qBbIlhiV2WquZ+LreOX2O8XBgMX3kbuzZFCu/VZSB0zzf2kUUKbI11uFxBk+Y7MlAil5Ey7Ajvb2q0lcb877Q== SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: caviumnetworks.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Oct 2015 16:09:08.6555 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM3PR07MB2140 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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" > 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" > > >> 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 > >> > > > > 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 > >