From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D766EC433E2 for ; Tue, 8 Sep 2020 18:25:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A5C4B2080A for ; Tue, 8 Sep 2020 18:25:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731303AbgIHSZC (ORCPT ); Tue, 8 Sep 2020 14:25:02 -0400 Received: from vps0.lunn.ch ([185.16.172.187]:50582 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730748AbgIHSWZ (ORCPT ); Tue, 8 Sep 2020 14:22:25 -0400 Received: from andrew by vps0.lunn.ch with local (Exim 4.94) (envelope-from ) id 1kFiG4-00Dp7f-7W; Tue, 08 Sep 2020 20:22:20 +0200 Date: Tue, 8 Sep 2020 20:22:20 +0200 From: Andrew Lunn To: Lukasz Stelmach Cc: devicetree@vger.kernel.org, linux-samsung-soc@vger.kernel.org, b.zolnierkie@samsung.com, netdev@vger.kernel.org, Russell King , Krzysztof Kozlowski , linux-kernel@vger.kernel.org, Rob Herring , Kukjin Kim , Jakub Kicinski , "David S. Miller" , linux-arm-kernel@lists.infradead.org, m.szyprowski@samsung.com Subject: Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver Message-ID: <20200908182220.GA3290129@lunn.ch> References: <20200907181854.GD3254313@lunn.ch> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Sender: linux-samsung-soc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-samsung-soc@vger.kernel.org On Tue, Sep 08, 2020 at 07:49:20PM +0200, Lukasz Stelmach wrote: > It was <2020-09-07 pon 20:18>, when Andrew Lunn wrote: > >> > On Tue, Aug 25, 2020 at 07:03:09PM +0200, Łukasz Stelmach wrote: > >> >> +++ b/drivers/net/ethernet/asix/ax88796c_ioctl.c > >> > > >> > This is an odd filename. The ioctl code is wrong anyway, but there is > >> > a lot more than ioctl in here. I suggest you give it a new name. > >> > > >> > >> Sure, any suggestions? > > > > Sorry, i have forgotten what is actually contained. > > IOCTL handler (.ndo_do_ioctl), ethtool ops, and a bunch of hw control > functions. > > > Does it even need to be a separate file? > > It doesn't need, but I think it makes sense to keep ioctl and ethtool > stuff in a separate file. Some of the hw control function look like they > might change after using phylib. _ethtool.c is a common file name. > Good point. I need to figure out how to do it. Can you point (from the > top fou your head) a driver which does it for a simmilarly integrated > device? Being integrated or not makes no difference. The API usage is the same. drivers/net/usb/smsc95xx.c has an integrated PHY i think. > I am not arguing to keep the parameter at any cost, but I would really > like to know if there is a viable alternative for DT and ACPI. This chip > is for smaller systems which not necessarily implement advanced > bootloaders (and DT). What bootload is being used, if not uboot or bearbox? > According to module.h > > /* > * Author(s), use "Name " or just "Name", for multiple > * authors use multiple MODULE_AUTHOR() statements/lines. > */ Thanks, did not know that. > >> >> + struct net_device *ndev = ax_local->ndev; > >> >> + int status; > >> >> + > >> >> + do { > >> >> + if (!(ax_local->checksum & AX_RX_CHECKSUM)) > >> >> + break; > >> >> + > >> >> + /* checksum error bit is set */ > >> >> + if ((rxhdr->flags & RX_HDR3_L3_ERR) || > >> >> + (rxhdr->flags & RX_HDR3_L4_ERR)) > >> >> + break; > >> >> + > >> >> + if ((rxhdr->flags & RX_HDR3_L4_TYPE_TCP) || > >> >> + (rxhdr->flags & RX_HDR3_L4_TYPE_UDP)) { > >> >> + skb->ip_summed = CHECKSUM_UNNECESSARY; > >> >> + } > >> >> + } while (0); > >> > > >> > > >> > ?? > >> > > >> > >> if() break; Should I use goto? > > > > Sorry, i was too ambiguous. Why: > > > > do { > > } while (0); > > > > It is an odd construct. > > As to "why" — you have correctly spotted, this is a vendor driver I am > porting. Although it's not like I am trying to avoid any changes, but > because this driver worked for us on older kernels (v3.10.9) I am trying > not to touch pieces which IMHO are good enough. My experience with vendor drivers is you nearly end up rewriting it to bring it up to mainline standards. I would suggest first setting up some automated tests, and then make lots of small changes, committed to git. You can then go backwards and find where regressions have been introduced and found by the automated tests. And then every so often squash it all together, ready for submission. > To avoid using do{}while(0) it requires either goto (instead of breaks), > nesting those if()s in one another or a humongous single if(). Neither > looks pretty and the last one is even less readable than > do()while. I removed too much context. Does anything useful happen afterwards? Maybe you can just use return? Or put that code into a helper which can use return rather than break? Andrew From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EFEDCC43461 for ; Tue, 8 Sep 2020 18:23:57 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9A3802080A for ; Tue, 8 Sep 2020 18:23:57 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="FL4gfA0l" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9A3802080A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=lunn.ch Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=U6K3wyJ4ubXXol1YjL20OujU0XsZDqU3YYFFUVEZN6o=; b=FL4gfA0licaWOFnErFEVUUYBq wt4zdyDBb2uZaftfPEWnvXBdahZdU4VHqwBmTkD8W9kaK4Cd0w1su7uBc2JckeSzQWsyrZ7PU7UPw bhVQybV7Eme889Dp36eFduWlGyRd6tEcESl/l5FUJq8hb4UW7mpFLumepfu0FGSza+kv6OCh+q18O eIh1O6H+e7HdJmot5MfQ7vY3zYOgYOhOAy4u7wrNA4dcTChWu5g3MgEWiRU3slBZvB8swfpU2s536 Tmc6nwtXyoZzCUgFcMvMwif4JjjKVBFh0YlS/EDUtLNhLLWe/rWb7mofKRwf1o/LNnkp/GBU5BlMP YX5DQx6IQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kFiGJ-0004bl-Uz; Tue, 08 Sep 2020 18:22:36 +0000 Received: from vps0.lunn.ch ([185.16.172.187]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kFiGG-0004aR-Fg for linux-arm-kernel@lists.infradead.org; Tue, 08 Sep 2020 18:22:33 +0000 Received: from andrew by vps0.lunn.ch with local (Exim 4.94) (envelope-from ) id 1kFiG4-00Dp7f-7W; Tue, 08 Sep 2020 20:22:20 +0200 Date: Tue, 8 Sep 2020 20:22:20 +0200 From: Andrew Lunn To: Lukasz Stelmach Subject: Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver Message-ID: <20200908182220.GA3290129@lunn.ch> References: <20200907181854.GD3254313@lunn.ch> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200908_142232_552202_1682FE4E X-CRM114-Status: GOOD ( 32.86 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, linux-samsung-soc@vger.kernel.org, b.zolnierkie@samsung.com, netdev@vger.kernel.org, Russell King , Krzysztof Kozlowski , linux-kernel@vger.kernel.org, Rob Herring , Kukjin Kim , Jakub Kicinski , "David S. Miller" , linux-arm-kernel@lists.infradead.org, m.szyprowski@samsung.com Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org T24gVHVlLCBTZXAgMDgsIDIwMjAgYXQgMDc6NDk6MjBQTSArMDIwMCwgTHVrYXN6IFN0ZWxtYWNo IHdyb3RlOgo+IEl0IHdhcyA8MjAyMC0wOS0wNyBwb24gMjA6MTg+LCB3aGVuIEFuZHJldyBMdW5u IHdyb3RlOgo+ID4+ID4gT24gVHVlLCBBdWcgMjUsIDIwMjAgYXQgMDc6MDM6MDlQTSArMDIwMCwg xYF1a2FzeiBTdGVsbWFjaCB3cm90ZToKPiA+PiA+PiArKysgYi9kcml2ZXJzL25ldC9ldGhlcm5l dC9hc2l4L2F4ODg3OTZjX2lvY3RsLmMKPiA+PiA+Cj4gPj4gPiBUaGlzIGlzIGFuIG9kZCBmaWxl bmFtZS4gVGhlIGlvY3RsIGNvZGUgaXMgd3JvbmcgYW55d2F5LCBidXQgdGhlcmUgaXMKPiA+PiA+ IGEgbG90IG1vcmUgdGhhbiBpb2N0bCBpbiBoZXJlLiBJIHN1Z2dlc3QgeW91IGdpdmUgaXQgYSBu ZXcgbmFtZS4KPiA+PiA+Cj4gPj4gCj4gPj4gU3VyZSwgYW55IHN1Z2dlc3Rpb25zPwo+ID4KPiA+ IFNvcnJ5LCBpIGhhdmUgZm9yZ290dGVuIHdoYXQgaXMgYWN0dWFsbHkgY29udGFpbmVkLiAKPiAK PiBJT0NUTCBoYW5kbGVyICgubmRvX2RvX2lvY3RsKSwgZXRodG9vbCBvcHMsIGFuZCBhIGJ1bmNo IG9mIGh3IGNvbnRyb2wKPiBmdW5jdGlvbnMuCj4gCj4gPiBEb2VzIGl0IGV2ZW4gbmVlZCB0byBi ZSBhIHNlcGFyYXRlIGZpbGU/Cj4gCj4gSXQgZG9lc24ndCBuZWVkLCBidXQgSSB0aGluayBpdCBt YWtlcyBzZW5zZSB0byBrZWVwIGlvY3RsIGFuZCBldGh0b29sCj4gc3R1ZmYgaW4gYSBzZXBhcmF0 ZSBmaWxlLiBTb21lIG9mIHRoZSBodyBjb250cm9sIGZ1bmN0aW9uIGxvb2sgbGlrZSB0aGV5Cj4g bWlnaHQgY2hhbmdlIGFmdGVyIHVzaW5nIHBoeWxpYi4KCjxkcml2ZXI+X2V0aHRvb2wuYyBpcyBh IGNvbW1vbiBmaWxlIG5hbWUuCgo+IEdvb2QgcG9pbnQuIEkgbmVlZCB0byBmaWd1cmUgb3V0IGhv dyB0byBkbyBpdC4gQ2FuIHlvdSBwb2ludCAoZnJvbSB0aGUKPiB0b3AgZm91IHlvdXIgaGVhZCkg YSBkcml2ZXIgd2hpY2ggZG9lcyBpdCBmb3IgYSBzaW1taWxhcmx5IGludGVncmF0ZWQKPiBkZXZp Y2U/CgpCZWluZyBpbnRlZ3JhdGVkIG9yIG5vdCBtYWtlcyBubyBkaWZmZXJlbmNlLiBUaGUgQVBJ IHVzYWdlIGlzIHRoZQpzYW1lLiBkcml2ZXJzL25ldC91c2Ivc21zYzk1eHguYyBoYXMgYW4gaW50 ZWdyYXRlZCBQSFkgaSB0aGluay4KCj4gSSBhbSBub3QgYXJndWluZyB0byBrZWVwIHRoZSBwYXJh bWV0ZXIgYXQgYW55IGNvc3QsIGJ1dCBJIHdvdWxkIHJlYWxseQo+IGxpa2UgdG8ga25vdyBpZiB0 aGVyZSBpcyBhIHZpYWJsZSBhbHRlcm5hdGl2ZSBmb3IgRFQgYW5kIEFDUEkuIFRoaXMgY2hpcAo+ IGlzIGZvciBzbWFsbGVyIHN5c3RlbXMgd2hpY2ggbm90IG5lY2Vzc2FyaWx5IGltcGxlbWVudCBh ZHZhbmNlZAo+IGJvb3Rsb2FkZXJzIChhbmQgRFQpLgoKV2hhdCBib290bG9hZCBpcyBiZWluZyB1 c2VkLCBpZiBub3QgdWJvb3Qgb3IgYmVhcmJveD8KCj4gQWNjb3JkaW5nIHRvIG1vZHVsZS5oCj4g Cj4gLyoKPiAgKiBBdXRob3IocyksIHVzZSAiTmFtZSA8ZW1haWw+IiBvciBqdXN0ICJOYW1lIiwg Zm9yIG11bHRpcGxlCj4gICogYXV0aG9ycyB1c2UgbXVsdGlwbGUgTU9EVUxFX0FVVEhPUigpIHN0 YXRlbWVudHMvbGluZXMuCj4gICovCgpUaGFua3MsIGRpZCBub3Qga25vdyB0aGF0Lgo+ID4+ID4+ ICsJc3RydWN0IG5ldF9kZXZpY2UgKm5kZXYgPSBheF9sb2NhbC0+bmRldjsKPiA+PiA+PiArCWlu dCBzdGF0dXM7Cj4gPj4gPj4gKwo+ID4+ID4+ICsJZG8gewo+ID4+ID4+ICsJCWlmICghKGF4X2xv Y2FsLT5jaGVja3N1bSAmIEFYX1JYX0NIRUNLU1VNKSkKPiA+PiA+PiArCQkJYnJlYWs7Cj4gPj4g Pj4gKwo+ID4+ID4+ICsJCS8qIGNoZWNrc3VtIGVycm9yIGJpdCBpcyBzZXQgKi8KPiA+PiA+PiAr CQlpZiAoKHJ4aGRyLT5mbGFncyAmIFJYX0hEUjNfTDNfRVJSKSB8fAo+ID4+ID4+ICsJCSAgICAo cnhoZHItPmZsYWdzICYgUlhfSERSM19MNF9FUlIpKQo+ID4+ID4+ICsJCQlicmVhazsKPiA+PiA+ PiArCj4gPj4gPj4gKwkJaWYgKChyeGhkci0+ZmxhZ3MgJiBSWF9IRFIzX0w0X1RZUEVfVENQKSB8 fAo+ID4+ID4+ICsJCSAgICAocnhoZHItPmZsYWdzICYgUlhfSERSM19MNF9UWVBFX1VEUCkpIHsK PiA+PiA+PiArCQkJc2tiLT5pcF9zdW1tZWQgPSBDSEVDS1NVTV9VTk5FQ0VTU0FSWTsKPiA+PiA+ PiArCQl9Cj4gPj4gPj4gKwl9IHdoaWxlICgwKTsKPiA+PiA+Cj4gPj4gPgo+ID4+ID4gPz8KPiA+ PiA+Cj4gPj4gCj4gPj4gaWYoKSBicmVhazsgU2hvdWxkIEkgdXNlIGdvdG8/Cj4gPgo+ID4gU29y cnksIGkgd2FzIHRvbyBhbWJpZ3VvdXMuIFdoeToKPiA+Cj4gPiBkbyB7Cj4gPiB9IHdoaWxlICgw KTsKPiA+Cj4gPiBJdCBpcyBhbiBvZGQgY29uc3RydWN0Lgo+IAo+IEFzIHRvICJ3aHkiIOKAlCB5 b3UgaGF2ZSBjb3JyZWN0bHkgc3BvdHRlZCwgdGhpcyBpcyBhIHZlbmRvciBkcml2ZXIgSSBhbQo+ IHBvcnRpbmcuIEFsdGhvdWdoIGl0J3Mgbm90IGxpa2UgSSBhbSB0cnlpbmcgdG8gYXZvaWQgYW55 IGNoYW5nZXMsIGJ1dAo+IGJlY2F1c2UgdGhpcyBkcml2ZXIgd29ya2VkIGZvciB1cyBvbiBvbGRl ciBrZXJuZWxzICh2My4xMC45KSBJIGFtIHRyeWluZwo+IG5vdCB0byB0b3VjaCBwaWVjZXMgd2hp Y2ggSU1ITyBhcmUgZ29vZCBlbm91Z2guCgpNeSBleHBlcmllbmNlIHdpdGggdmVuZG9yIGRyaXZl cnMgaXMgeW91IG5lYXJseSBlbmQgdXAgcmV3cml0aW5nIGl0IHRvCmJyaW5nIGl0IHVwIHRvIG1h aW5saW5lIHN0YW5kYXJkcy4gSSB3b3VsZCBzdWdnZXN0IGZpcnN0IHNldHRpbmcgdXAKc29tZSBh dXRvbWF0ZWQgdGVzdHMsIGFuZCB0aGVuIG1ha2UgbG90cyBvZiBzbWFsbCBjaGFuZ2VzLCBjb21t aXR0ZWQKdG8gZ2l0LiBZb3UgY2FuIHRoZW4gZ28gYmFja3dhcmRzIGFuZCBmaW5kIHdoZXJlIHJl Z3Jlc3Npb25zIGhhdmUgYmVlbgppbnRyb2R1Y2VkIGFuZCBmb3VuZCBieSB0aGUgYXV0b21hdGVk IHRlc3RzLiBBbmQgdGhlbiBldmVyeSBzbyBvZnRlbgpzcXVhc2ggaXQgYWxsIHRvZ2V0aGVyLCBy ZWFkeSBmb3Igc3VibWlzc2lvbi4KCj4gVG8gYXZvaWQgdXNpbmcgZG97fXdoaWxlKDApIGl0IHJl cXVpcmVzIGVpdGhlciBnb3RvIChpbnN0ZWFkIG9mIGJyZWFrcyksCj4gbmVzdGluZyB0aG9zZSBp ZigpcyBpbiBvbmUgYW5vdGhlciBvciBhIGh1bW9uZ291cyBzaW5nbGUgaWYoKS4gTmVpdGhlcgo+ IGxvb2tzIHByZXR0eSBhbmQgdGhlIGxhc3Qgb25lIGlzIGV2ZW4gbGVzcyByZWFkYWJsZSB0aGFu Cj4gZG8oKXdoaWxlLgoKSSByZW1vdmVkIHRvbyBtdWNoIGNvbnRleHQuIERvZXMgYW55dGhpbmcg dXNlZnVsIGhhcHBlbiBhZnRlcndhcmRzPwpNYXliZSB5b3UgY2FuIGp1c3QgdXNlIHJldHVybj8g T3IgcHV0IHRoYXQgY29kZSBpbnRvIGEgaGVscGVyIHdoaWNoCmNhbiB1c2UgcmV0dXJuIHJhdGhl ciB0aGFuIGJyZWFrPwoKICAgICAgQW5kcmV3CgpfX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1h cm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcv bWFpbG1hbi9saXN0aW5mby9saW51eC1hcm0ta2VybmVsCg==