Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753012AbcK0BCw (ORCPT ); Sat, 26 Nov 2016 20:02:52 -0500 Received: from mout.gmx.net ([212.227.17.22]:65200 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750954AbcK0BCv (ORCPT ); Sat, 26 Nov 2016 20:02:51 -0500 Subject: Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver To: Rami Rosen References: <1480162850-8014-1-git-send-email-LinoSanfilippo@gmx.de> <1480162850-8014-2-git-send-email-LinoSanfilippo@gmx.de> Cc: David Miller , charrer@alacritech.com, liodot@gmail.com, gregkh@linuxfoundation.org, andrew@lunn.ch, devel@driverdev.osuosl.org, "linux-kernel@vger.kernel.org" , Netdev From: Lino Sanfilippo Message-ID: <48c6a5d1-cde3-6e23-9c6c-d26838fce332@gmx.de> Date: Sun, 27 Nov 2016 02:02:27 +0100 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Provags-ID: V03:K0:nMnUgV1c2JpOptv/TtlI5aQvRAto4sPxIAInHuq4SFdpp/i8Tiw CtK42bgeIuIDOAjxz79q+bitdXGX9ZCPpnXJCWglfHlGagNJH5pS1buCOwU9KGFy9FrG7NV NaWPtUJ7j0S0/Gbq+Tfl9/WH5brloEVnhmB14mmj7sDQIf30dasaC/MBp8GDPs+v4vB4fR/ 9COb8o5h6/aQqvqNilTJQ== X-UI-Out-Filterresults: notjunk:1;V01:K0:ODQHsJODCyg=:X6xpetTNhoq+CVfK8Kg9/A YDy1xpXiHwcvsngP2T7akDAGscf4cFkjJ5ekt3zc/koEW9eQaTSiSBmxLysgWx9NQSQLm2ogm B4lEl7iSk7jdTxJ96fi/ZqsPuZGbCo0N0tD5vGwYxNBJzOFRc6bOtZ/qTCFrYe4D/7+zZkvcl G3ZjPsJ4dCmC0S98Cnh9/ldzapQY3X7ks0AE9d+mxEPQLz17EzeVMp29eVUI/IVipISz7GRxS I6NQiUVjv6qzaeRZ8FyYxb4DOi6zkh55TqdyLHj1J4+/bvYcdYc816zSnrlN+DxmvrNH1sTrH 7P8flkIHQOURt+02Ed55ePmzrvTUl6lIFaRQ/24yNfwV3z+6U1Ct4ooi3hHY9nWJOjQqhqI8y W03/RrXCy0vdF7C8yWy+2903LsmpoBHzF3Z5rxfCq7767f1K60N/CgbLGHWyRUzYPm7nKPTs5 9/dL81zELLdpiiU+jSw50czA+jf8+XB7yqWKwDaW2F3+Mqp5ShspgrBVrLMGyPYjJCgsbvF/P LafjCqQXfHFAMRJSWQiCsQ5iCGstMAl89xlf2fAvvv2hZZHQwz0hRt4gS/pP/E3qu2EKz/8SM aHxWh1dcpq3zBPYEKYOrMbFgmh+BS1VY4D2D9y15JwrvXtyry/3Lm7Qzf52S+KkhA8isEMc+2 N0u/LDdYy8PHEHVxg9VedJjN1JHqON0X6Chy9nTAld6SHdrtUNSDusbE+9cmLSprgIVB0M7RN ddiHeRiyuvt/KjUW+R0uDR7O0CxBh5xNsYMxd0buppZ30EayFdSK6AMHyXgJuz8E+yYFXOYy3 vgSmi0A Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2583 Lines: 96 Hi Rami, On 26.11.2016 16:48, Rami Rosen wrote: >> @@ -0,0 +1,28 @@ >> +config NET_VENDOR_ALACRITECH >> + bool "Alacritech devices" >> + default y >> + ---help--- >> + If you have a network (Ethernet) card belonging to this class, say Y. >> + >> + Note that the answer to this question doesn't directly affect the >> + kernel: saying N will just cause the configurator to skip all > > Shouldn't it be "Alacritech devices" here, as appears earlier ? > >> + the questions about Renesas devices. If you say Y, you will be asked Yes, it definitely should not be Renesas :). This is a stupid copy and paste error, I will fix it, thank you! >> + for your specific device in the following questions. >> + > > ... > ... > ... >> +struct slic_device { >> + struct pci_dev *pdev; > ... >> + bool promisc; > > Seems that the autoneg boolean is not used anywhere, apart from > setting it once to true in > the slic_set_link_autoneg() method. Apart from this member it is not > accessed anywhere, so it seems it should be removed. > >> + bool autoneg; >> + int speed; Agreed, this variable can be removed. > ... > >> +static int slic_load_rcvseq_firmware(struct slic_device *sdev) >> +{ >> + const struct firmware *fw; >> + const char *file; >> + u32 codelen; >> + int idx = 0; >> + u32 instr; >> + u32 addr; >> + int err; >> + > ... >> + /* Do an initial sanity check concerning firmware size now. A further >> + * check follows below. >> + */ >> + if (fw->size < SLIC_FIRMWARE_MIN_SIZE) { >> + dev_err(&sdev->pdev->dev, >> + "invalid firmware size %zu (min %u expected)\n", >> + fw->size, SLIC_FIRMWARE_MIN_SIZE); >> + err = -EINVAL; > > in the release label, always 0 is returned: > >> + goto release; >> + } >> + >> + codelen = slic_read_dword_from_firmware(fw, &idx); >> + >> + /* do another sanity check against firmware size */ >> + if ((codelen + 4) > fw->size) { >> + dev_err(&sdev->pdev->dev, >> + "invalid rcv-sequencer firmware size %zu\n", fw->size); >> + err = -EINVAL; > > Again, in the release label, always 0 is returned: > >> + goto release; >> + } >> + >> >> +release: >> + release_firmware(fw); >> + >> + return 0; >> +} This should return "err", I will fix it. Thanks a lot for the review! Regards, Lino