Re: [PATCH v2 2/3] net: qrtr: Add MHI transport layer

Re: [PATCH v2 2/3] net: qrtr: Add MHI transport layer

[Markus Elfring]

> Hence, this commit adds MHI transport layer support to QRTR for
> transferring the QMI messages over IPC Router.

I suggest to reconsider software development consequences around
another implementation detail.


…
> +static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb)
> +{
…
> +	rc = mhi_queue_skb(qdev->mhi_dev, DMA_TO_DEVICE, skb, skb->len,
> +			   MHI_EOT);
> +	if (rc) {
> +		kfree_skb(skb);
> +		return rc;
> +	}
…
> +}

I propose again to add a jump target so that a bit of exception handling code
can be better reused at the end of this function implementation.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=b2768df24ec400dd4f7fa79542f797e904812053#n450

+	if (rc)
+		goto free_skb;
…
+	return rc;
+
+free_skb:
+	kfree_skb(skb);
+	return rc;
+}


Regards,
Markus

Re: [PATCH v2 2/3] net: qrtr: Add MHI transport layer

[Manivannan Sadhasivam]

On Sun, Apr 26, 2020 at 03:45:41PM +0200, Markus Elfring wrote:
> > Hence, this commit adds MHI transport layer support to QRTR for
> > transferring the QMI messages over IPC Router.
> 
> I suggest to reconsider software development consequences around
> another implementation detail.
> 
> 
> …
> > +static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb)
> > +{
> …
> > +	rc = mhi_queue_skb(qdev->mhi_dev, DMA_TO_DEVICE, skb, skb->len,
> > +			   MHI_EOT);
> > +	if (rc) {
> > +		kfree_skb(skb);
> > +		return rc;
> > +	}
> …
> > +}
> 
> I propose again to add a jump target so that a bit of exception handling code
> can be better reused at the end of this function implementation.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=b2768df24ec400dd4f7fa79542f797e904812053#n450
> 

Matter of taste! goto's are really useful if there are multiple exit paths
available. But in this case there is only one and I don't think we may add
anymore in future. So I'll keep it as it is.

Thanks,
Mani

> +	if (rc)
> +		goto free_skb;
> …
> +	return rc;
> +
> +free_skb:
> +	kfree_skb(skb);
> +	return rc;
> +}
> 
> 
> Regards,
> Markus

Re: [v2 2/3] net: qrtr: Add MHI transport layer

[Markus Elfring]

>> I propose again to add a jump target so that a bit of exception handling code
>> can be better reused at the end of this function implementation.
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=b2768df24ec400dd4f7fa79542f797e904812053#n450
>>
>
> Matter of taste! goto's are really useful if there are multiple exit paths
> available. But in this case there is only one and I don't think we may add
> anymore in future. So I'll keep it as it is.

Do you hope that an other optimiser software will avoid duplicate code
like kfree_skb(skb) calls from if branches?

Regards,
Markus

Re: [v2 2/3] net: qrtr: Add MHI transport layer

[Manivannan Sadhasivam]

On 27 April 2020 12:20:43 PM IST, Markus Elfring <Markus.Elfring@web.de> wrote:
>>> I propose again to add a jump target so that a bit of exception
>handling code
>>> can be better reused at the end of this function implementation.
>>>
>https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=b2768df24ec400dd4f7fa79542f797e904812053#n450
>>>
>>
>> Matter of taste! goto's are really useful if there are multiple exit
>paths
>> available. But in this case there is only one and I don't think we
>may add
>> anymore in future. So I'll keep it as it is.
>
>Do you hope that an other optimiser software will avoid duplicate code
>like kfree_skb(skb) calls from if branches?
>

Doh. I didn't notice the previous kfree in skb_linearize(). Will spin v3 incorporating this change. 

Thanks, 
Mani

>Regards,
>Markus

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.