* Re: [PATCH v2 2/3] net: qrtr: Add MHI transport layer
@ 2020-04-26 13:45 Markus Elfring
2020-04-27 5:40 ` Manivannan Sadhasivam
0 siblings, 1 reply; 4+ messages in thread
From: Markus Elfring @ 2020-04-26 13:45 UTC (permalink / raw)
To: Manivannan Sadhasivam, Chris Lew, Bjorn Andersson, netdev, linux-arm-msm
Cc: linux-kernel, kernel-janitors, David S. Miller,
Greg Kroah-Hartman, Hemant Kumar, Jeffrey Hugo, Kalle Valo,
Siddartha Mohanadoss
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 2/3] net: qrtr: Add MHI transport layer
2020-04-26 13:45 [PATCH v2 2/3] net: qrtr: Add MHI transport layer Markus Elfring
@ 2020-04-27 5:40 ` Manivannan Sadhasivam
2020-04-27 6:50 ` [v2 " Markus Elfring
0 siblings, 1 reply; 4+ messages in thread
From: Manivannan Sadhasivam @ 2020-04-27 5:40 UTC (permalink / raw)
To: Markus Elfring
Cc: Chris Lew, Bjorn Andersson, netdev, linux-arm-msm, linux-kernel,
kernel-janitors, David S. Miller, Greg Kroah-Hartman,
Hemant Kumar, Jeffrey Hugo, Kalle Valo, Siddartha Mohanadoss
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [v2 2/3] net: qrtr: Add MHI transport layer
2020-04-27 5:40 ` Manivannan Sadhasivam
@ 2020-04-27 6:50 ` Markus Elfring
2020-04-27 6:57 ` Manivannan Sadhasivam
0 siblings, 1 reply; 4+ messages in thread
From: Markus Elfring @ 2020-04-27 6:50 UTC (permalink / raw)
To: Manivannan Sadhasivam, Chris Lew, Bjorn Andersson, netdev, linux-arm-msm
Cc: linux-kernel, kernel-janitors, David S. Miller,
Greg Kroah-Hartman, Hemant Kumar, Jeffrey Hugo, Kalle Valo,
Siddartha Mohanadoss
>> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [v2 2/3] net: qrtr: Add MHI transport layer
2020-04-27 6:50 ` [v2 " Markus Elfring
@ 2020-04-27 6:57 ` Manivannan Sadhasivam
0 siblings, 0 replies; 4+ messages in thread
From: Manivannan Sadhasivam @ 2020-04-27 6:57 UTC (permalink / raw)
To: Markus Elfring, Chris Lew, Bjorn Andersson, netdev, linux-arm-msm
Cc: linux-kernel, kernel-janitors, David S. Miller,
Greg Kroah-Hartman, Hemant Kumar, Jeffrey Hugo, Kalle Valo,
Siddartha Mohanadoss
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-27 6:57 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-26 13:45 [PATCH v2 2/3] net: qrtr: Add MHI transport layer Markus Elfring
2020-04-27 5:40 ` Manivannan Sadhasivam
2020-04-27 6:50 ` [v2 " Markus Elfring
2020-04-27 6:57 ` Manivannan Sadhasivam
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).