On Fri, Oct 25, 2024 at 06:00:42AM +0300, Wei Fang wrote:
> > Don't artificially create errors when there are really no errors to handle.
> > Both enetc_pf_ops and enetc4_pf_ops provide .set_si_primary_mac(), so it
> > is unnecessary to handle the case where it isn't present. Those functions
> > return void, and void can be propagated to enetc_set_si_hw_addr() as well.
>
> I thought checking the pointer is safer, so you mean that pointers that are
> definitely present in the current driver do not need to be checked?
Yes, there is no point to check for a condition which is impossible
to trigger in the current code. The callee and the caller are tightly
coupled (in the same driver), it's not a rigid API, so if the function
pointers should be made optional for some future hardware IP, the error
handling will be added when necessary. Ideally any change passes through
review, and any inconsistency should be spotted when added.
> > A bit inconsistent that pf->ops->set_si_primary_mac() goes through a
> > wrapper function but this doesn't.
> >
>
> If we really do not need to check these callback pointers, then I think I can
> remove the wrapper.
Fine without wrapping throughout, my comment was first and foremost
about consistency.
> > This one looks extremely weird in the existing code, but I suppose I'm
> > too late to the party to request you to clean up any of the PSFP code,
> > so I'll make a note to do it myself after your work. I haven't spotted
> > any actual bug, just weird coding patterns.
> >
> > No change request here. I see the netc4_pf doesn't implement enable_psfp(),
> > so making it optional here is fine.
>
> Yes, PSFP is not supported in this patch set, I will remove it in future.
If by "I will remove it in future" you mean "once NETC4 gains PSFP
support, I will make enable_psfp() non-optional", then ok, great.
> Currently, we have not add the PCS support, so the 10G ENETC is not supported
> yet. And we also disable the 10G ENETC in DTS. Only the 1G ENETCs (without PCS)
> are supported for i.MX95.
Also think about the case where the current version of the kernel
will boot on a newer version of the device tree, which does not have
'status = "disabled";' for those ports. It should do something reasonable.
In any case, "they're now disabled in the device tree" is not an argument.
My anecdotal and vague understanding of the Arm SystemReady (IR I think)
requirements is that the device tree is provided by the platform,
separately from the kernel/rootfs. It relies on the device tree ABI
being stable, backwards-compatible and forwards-compatible.
> > A message maybe, stating what's the deal? Just that users figure out
> > quickly that it's an expected behavior, and not spend hours debugging
> > until they find out it's not their fault?
>
> I will explain in the commit message that i.MX95 10G ENETC is not currently
> supported.
By "a message, maybe" I actually meant dev_err("Message here\n"); rather
than silent/imprecise failure.
