Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver

From: Lino Sanfilippo
Date: Sat Nov 26 2016 - 20:03:06 EST



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