On 2/27/2024 8:10 AM, Krishna Kurapati PSSNV wrote:
In general this is of course fine (though see Greg's auto-complaint).
I haven't thought too much about this, but I just wonder whether the
check for block_len == 0
shouldn't be just after block_len is read, ie. somewhere just after:
block_len = get_ncm(&tmp, opts->block_length);
as it is kind of weird to be handling block_len == 0 at the point where
you are already theoretically done processing the block...
I guess, as is, this assumes the block isn't actually of length 0,
since there's a bunch of following get_ncm() calls...
Are those guaranteed to be valid?
I did get this doubt and tried it. I bailed out as soon as I found out
block len is zero without actually processing the datagrams present and
when I did that even ping doesn't work. Everything works only when the
datagrams in this zero block len NTB are parsed properly.
I guess I don't actually see the infinite loop with block_len == 0,
since get_ncm() always moves us forward...
The infinite loop occurs because we keep moving the buffer pointer
forward and keep processing the giveback until to_process variable
becomes zero or one. In case block length is zero, we never move the
buffer pointer forward and never reduce to_process variable and hence
keep infinitely processing the same NTB over and over again.
Maybe your patch *is* correct as is, and you just need a comment
explaining *why* block_len == 0 is terminal at the spot you're adding
the check.
Also couldn't you fix this without goto, by changing
} else if (to_process > 0) {
to
} else if (to_process && block_len) {
// See NCM spec. zero block_len means short packet.
I will test this out once (although I know that looking at it, it would
definitely work) and send v2 with this diff.
Thanks for the review.
Hi Maciej, Greg,
Thanks for approving v2.
Not sure if this is the right forum to ask this question, but had one
query. In the NCM driver, the register_netdev is called during bind but
the cleanup for that is called during free_inst. Meaning if usb0
interface is created for ncm on bind or a composition switch into NCM
(first comp switch after bootup), then it is removed only after removing
the entire g1/functions/ncm.0 folder.
Shouldn't we cleanup and remove the usb0 interface in unbind as a
counter operation of bind ? By extension this question also applies to
f_eem/ f_ecm/ f_rndis where it was done in similar manner. So was
wondering if anyone could help me with info on why it was designed that way.
Regards,
Krishna,
