Hi Dan.
I absolutely agree with all of your comments and I appreciate your review.
I agree that all of the issues you've pointed out, with the the exception of
one, need to be addressed. The issues pointed out - especially the string
manipulation issues - can turn into CVEs. We don't want to be checking bugs
like this into Linux. Certainly, nothing should be merged that does not
pass the static checker, et al, automated tools we have.
My comment here was only to say that I don't think it's reasonable to
ask Karan to break this change into a series of 100 small, reviewable changes.
To explain: the fnic driver is Cisco's driver. Cisco has always taken responsibility
for this driver and Karan is one of the many Cisco Maintainers. What's happening is,
after a long period of time where the code has been somewhat neglected, Cisco has
decided to do a major update of their upstream driver. These changes
are really more like a new driver, with some major new features, than a
series of bug fixes or updates. This is happening because - at the insistence
of some of the Distros, like Red Hat - Cisco is being told they need to bring
their upstream drivers in line with their out-of-box drivers.
Karan can confirm if this is the case. My question for Karan is: are there any more
major driver changes coming for fnic, or is this the majority of it?
If this were simply a new feature or a series of bug fixes I agree the changes
need to be organized into a series of small, reviewable patches. However,
under the circumstances, I am willing to hold my nose and say: just get your driver
updated and into order, and then we will hold the Maintainers (not Martin) accountable.
So I guess I am recommending an exception to the rule, just this once, in regard to breaking the
overall change into a smaller and smaller patch series. I would prefer, instead, that any and all
changes needed to address further review comments be presented as a small series of patches, broken
down into reviewable chunks, on top of the exiting patch series. It can be decided later if these
patches can or should be squashed into the respective 17 commits.
Anyways, those are my thoughts.
Thanks again for your help with this review. I agree Karan needs help and support,
and I will try to be more public about they ways I am doing that - which have been
mostly in the back-ground.
/John
On 1/14/25 04:59, Dan Carpenter wrote:
On Mon, Jan 13, 2025 at 12:35:03PM -0500, John Meneghini wrote:
Just a note to say that these patches are important to Red Hat and we
are actively engaged in back porting and testing these patches in to
RHEL-9 and RHEL-10.
I think these issues that Dan has pointed out are all issues which
can be addressed in a follow up patch.
I mean we already merged this. I only got involved because of static
checker issues in linux-next.
What I'm complaining about is not so much any specific issue but just
that the process was not followed. Normally this patch would not
be merged. If anyone sent a patch like this to drivers/staging it
would have triggered Greg's patch-bot automated response:
- Your patch did many different things all at once, making it difficult
to review. All Linux kernel patches need to only do one thing at a
time. If you need to do multiple things (such as clean up all coding
style issues in a file/driver), do it in a sequence of patches, each
one doing only one thing. This will make it easier to review the
patches to ensure that they are correct, and to help alleviate any
merge issues that larger patches can cause.
This rule is the same in every subsystem. No one wants to merge a patch
like this. But what happened is the patch sat on the list and only
Hannes and Martin were doing any review. Karan was left doing all the
work with no help or guidance. Eventually Martin has to give up right?
The patchset isn't up to normal standards but it's basically okay and
Martin can't do every single thing by himself and eventually it's pretty
clear no one else is coming to help. It is what it is etc.
Please understand this in the gentlest way. Next time if something is
important then assign an engineer to help out. It would have taken a day
to prepare this patchset for merging. You had seven months. It's not
fair to show up five days before the merge window asking for special
exemptions from the review process. Maintainers and reviewers are
already overworked, they shouldn't have to work around your deadlines.
From what I've seen Karan is absolutely doing a good job addressing the
problems I reported so it's fine. But normally this is not how it works.
regards,
dan carpenter
