On Tue, Mar 19, 2024 at 12:38:03PM +0100, Erwan Velu wrote:
>
> Le 18/03/2024 à 18:45, Simon Horman a écrit :
> > [...]
> > Hi Erwan, all,
> >
> > As a fix, I think this patch warrants a fixes tag.
> > Perhaps this one is appropriate?
> >
> > Fixes: 41c445ff0f48 ("i40e: main driver core")
>
> Simon
>
> Isn't that a bit too generic ?
Yes, maybe it is.
What we would be after is the first commit where the
user can hit the problem the patch addresses.
> [..]
>
> > I am fine with this patch, so please take what follows as a suggestion
> > for improvement, possibly as a follow-up. Not as a hard requirement from
> > my side.
> >
> > The part of this function between the two hunks of this patch is:
> >
> > netdev_err(netdev, "Error changing mtu to %d, Max is %d\n",
> > new_mtu, frame_size - I40E_PACKET_HDR_PAD);
> >
> > My reading is that with this patch two different limits are
> > checked wrt maximum MTU size:
> >
> > 1. A VSI level limit, which relates to RX buffer size
> > 2. A PHY level limit that relates to the MFS
> >
> > That seems fine to me. But the log message for 1 (above) does
> > not seem particularly informative wrt which limit has been exceeded.
>
> I got some comments around this.
>
> I wanted to keep my patch being focused on the mfs issue, but I can offer a
> patch to get a similar output for this. What WRT stands for ?
>
>
> I wanted also to make another patch for this :
>
> dev_warn(&pdev->dev, "MFS for port %x has been set below the default:
> %x\n",pf->hw.port, val);
>
> The MFS reported as hex without a "0x" prefix is very misleading, I can
> offer a patch for this too.
FWIIW, I think handling these questions in follow-up patches is fine.
