> 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.
>
For this case where the kernel is lower but the device tree is newer, I think
there is no problem. It just fails in probe() and does not affect other functions.
The old kernel does not support PCS, it is reasonable to return a '- EOPNOTSUPP'
error.
> 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.
Okay, I'll add an explicit error message.
