On Tue, Apr 3, 2018 at 11:52 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> On Fri, Mar 30, 2018 at 05:13:21PM +0200, Sergio Paracuellos wrote:
>> @@ -686,32 +673,61 @@ static int ks7010_upload_firmware(struct ks_sdio_card *card)
>> }
>> if (size == 0)
>> break;
>> +
>
> There are a few unrelated white space changes. This one is easy because
> it's a blank line
The unrelated changes are because code has been moved to a new
function and I reorder
here a bit the code. Because they are minor changes in the new
function I though it just
be ok to do this. Obviously it seems I was wrong :-).
>
>> memcpy(rom_buf, fw_entry->data + n, size);
>>
>> offset = n;
>> - ret = ks7010_sdio_update_index(priv, KS7010_IRAM_ADDRESS + offset);
>> + ret = ks7010_sdio_update_index(priv,
>> + KS7010_IRAM_ADDRESS + offset);
>
> but this one eats my review time to have to look at each letter to see
> what the difference is and how it relates to factoring out the function.
> I've marked all the unrelated white space changes. Please, move them to
> a different patch when you redo these.
>
>> if (ret)
>> - goto release_firmware;
>> + goto copy_error;
>
> Ideally, the label name would tell me what the goto does. This label
> name is based on the function name but that's useless because, of course
> the label is going to be in the same function as the goto. That's a
> given. A better name would be: "goto free_rom_buf;".
>
>>
>> ret = ks7010_sdio_write(priv, DATA_WINDOW, rom_buf, size);
>> if (ret)
>> - goto release_firmware;
>> + goto copy_error;
>>
>> - ret = ks7010_sdio_data_compare(priv, DATA_WINDOW, rom_buf, size);
>> + ret = ks7010_sdio_data_compare(priv,
>> + DATA_WINDOW, rom_buf, size);
>
> Unrelated change.
>
>> if (ret)
>> - goto release_firmware;
>> + goto copy_error;
>>
>> n += size;
>>
>> } while (size);
>>
>> - ret = ks7010_sdio_writeb(priv, GCR_A, GCR_A_REMAP);
>> + return ks7010_sdio_writeb(priv, GCR_A, GCR_A_REMAP);
>
> You're leaking "rom_buf" here. It needs to be:
>
> ret = ks7010_sdio_writeb(priv, GCR_A, GCR_A_REMAP);
True, thanks for points me out this. I fix this and the label and
resend v2 for this.
>
> free_rom_buf:
> kfree(rom_buf);
> return ret;
>
>> +
>> +copy_error:
>> + kfree(rom_buf);
>> + return ret;
>> +}
>> +
>> +static int ks7010_upload_firmware(struct ks_sdio_card *card)
>> +{
>> + struct ks_wlan_private *priv = card->priv;
>> + unsigned int n;
>> + int ret;
>> + const struct firmware *fw_entry = NULL;
>> +
>> + sdio_claim_host(card->func);
>> +
>> + if (ks7010_is_firmware_running(priv, &ret)) {
>> + netdev_dbg(priv->net_dev, "MAC firmware running ...\n");
>> + goto release_host;
>> + }
>> +
>> + ret = request_firmware(&fw_entry, ROM_FILE,
>> + &priv->ks_sdio_card->func->dev);
>> + if (ret)
>> + goto release_host;
>> +
>> + ret = ks7010_copy_firmware(priv, fw_entry);
>> if (ret)
>> goto release_firmware;
>>
>> /* Firmware running check */
>> for (n = 0; n < 50; ++n) {
>> - mdelay(10); /* wait_ms(10); */
>> + mdelay(10);
>
> Unrelated change.
Thanks for the review, Dan.
>
> regards,
> dan carpenter
Best regards,
Sergio Paracuellos
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
