Netdev List
 help / color / mirror / Atom feed
From: Adam Young <admiyo@amperemail.onmicrosoft.com>
To: Jeremy Kerr <jk@codeconstruct.com.au>,
	Adam Young <admiyo@os.amperecomputing.com>,
	Matt Johnston <matt@codeconstruct.com.au>,
	Andrew Lunn <andrew+netdev@lunn.ch>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Sudeep Holla <sudeep.holla@arm.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Huisong Li <lihuisong@huawei.com>
Subject: Re: [net-next v41] mctp pcc: Implement MCTP over PCC Transport
Date: Tue, 12 May 2026 12:38:16 -0400	[thread overview]
Message-ID: <aecf0ac6-0dcc-4b7c-bcd7-dd78082161a0@amperemail.onmicrosoft.com> (raw)
In-Reply-To: <71b7c8f40efc25fadb0aede6be888986364f2d53.camel@codeconstruct.com.au>


On 5/12/26 00:07, Jeremy Kerr wrote:
> Hi Adam,
>
>> Sorry, I should have been more explicit here.  I am not certain what is
>> going to happen with fragmentation, so I want to be protected against
>> future changes.
> Nothing is going to happen with fragmentation. Your driver will only see
> linear skbs in the ndo_start_xmit callback.
>
> Adding the skb_linearize() there is unnecessary, and creates ambiguity
> about the structure of the skbs that we're dealing with in that path.
>
OK, I get it now.  I was thinking that the Fragmentation was a likely 
part of the progression, as default MCTP packets are so small.



>> The check in validate_xmit_skb() is good, as it protects against the
>> current set up. So my option was to put a comment in here and hope both
>> changes happened together, or to just try and get this portion of the
>> driver solid against the change.
> I'm not clear on what changes you're referring to here?

Future proofing.  Sounds like it is a non-issue.  Disregard.  I will 
remove the linearize call in a future revision.


>
>> And I thought that was what you were suggesting in the comment. The
>> original comment sounded more like an "here is an optimization" instead
>> of "this is important enough to kick back"
> It's less of an optimisation, and more removing something that is
> unneeded, and potentially confusing ("why is the driver doing this?")?
>
>> As for spacing, I get that there is a style, but it really should be
>> encoded in checkstyle.sh or something and automated.  My own tendency is
>> to put way too many spaces in to chunk things together, and I end up
>> going over-draconian on stripping them out to try and meet the expected
>> layout.
> I'm not too fussed about the style at the level you have here, these are
> suggestions to clean up if you're already re-rolling. My confusion is
> that you had applied my feedback to one part of the code, but not to the
> area I had commented on (which has the same style structure).
>
> You also had some sashiko feedback on v40. I'm not sure whether all
> items are relevant (I *think* you're OK for the first, for example), but
> worth confirming:
>
> https://sashiko.dev/#/patchset/20260508032953.337036-1-admiyo%40os.amperecomputing.com

Yeah, I saw those, and was processing through them.  I have found 
similar type issues running against Codex.

Some of the issues I found are due to the interactions with the PCC 
layer, and need to be addressed there.  It leads to some edge conditions 
that are, for me, impossible to produce right now. There is one change 
it suggests which I thought I had already made.

If those changes are going to cause significatnt changes, I will 
probably also integrate your suggestion on how to deal with IRQ-safe 
stats posting.



> Cheers,
>
>
> Jeremy

  reply	other threads:[~2026-05-12 16:38 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-10 16:32 [net-next v41] mctp pcc: Implement MCTP over PCC Transport Adam Young
2026-05-11  2:19 ` Jeremy Kerr
2026-05-11 14:52   ` Adam Young
2026-05-12  4:07     ` Jeremy Kerr
2026-05-12 16:38       ` Adam Young [this message]
2026-05-13  1:22         ` Jeremy Kerr

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aecf0ac6-0dcc-4b7c-bcd7-dd78082161a0@amperemail.onmicrosoft.com \
    --to=admiyo@amperemail.onmicrosoft.com \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=admiyo@os.amperecomputing.com \
    --cc=andrew+netdev@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=jk@codeconstruct.com.au \
    --cc=kuba@kernel.org \
    --cc=lihuisong@huawei.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=matt@codeconstruct.com.au \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=sudeep.holla@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox