Re: [PATCH] Module Name: drivers/nfc/pn544/pn544.c fix a bug

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

On 25/12/2021 14:27, hrshy0629 wrote:
>     I noticed that the same usage for API nfc_hci_send_cmd() in line 541 and line 552 of the file pn544.c.
>     And the variable r is checked on line 545 but not checked on line 552.
>     The r in line 552 should be checked.

Why 'r' should be tested and returned early in line 552? Just because
some other code has slightly similar pattern, does not mean you should
apply it everywhere blindly.

Patch title and description is not matching coding style. Please, go via
git history to find how it is being written.

Title must describe what bug exactly you fix. Use imperative mode in the
commit description. Describe what is the bug and h ow it should be fixed.

> 
> Signed-off-by: hrshy0629 <hongqiang601217@gmail.com>
> ---
>  drivers/nfc/pn544/pn544.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/nfc/pn544/pn544.c b/drivers/nfc/pn544/pn544.c
> index 32a61a185142..531eda0d11a2 100644
> --- a/drivers/nfc/pn544/pn544.c
> +++ b/drivers/nfc/pn544/pn544.c
> @@ -552,6 +552,8 @@ static int pn544_hci_complete_target_discovered(struct nfc_hci_dev *hdev,
>  			r = nfc_hci_send_cmd(hdev, PN544_RF_READER_F_GATE,
>  					     PN544_RF_READER_CMD_ACTIVATE_NEXT,
>  					     uid_skb->data, uid_skb->len, NULL);
> +			if (r < 0)
> +				return r;

This looks like creating a leak, so NAK. Please provide explanation why
such error patch should be used.

>  			kfree_skb(uid_skb);


Best regards,
Krzysztof