Re: [Intel-wired-lan] [iwl-next v2 03/15] ice: add basic devlink subfunctions support

["Keller, Jacob E" <jacob.e.keller@intel.com>]

> -----Original Message-----
> From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Sent: Monday, May 13, 2024 4:44 AM
> To: Jiri Pirko <jiri@resnulli.us>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Keller, Jacob E
> <jacob.e.keller@intel.com>; Kubiak, Michal <michal.kubiak@intel.com>;
> Fijalkowski, Maciej <maciej.fijalkowski@intel.com>; Samudrala, Sridhar
> <sridhar.samudrala@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Drewek, Wojciech
> <wojciech.drewek@intel.com>; pio.raczynski@gmail.com; jiri@nvidia.com;
> Polchlopek, Mateusz <mateusz.polchlopek@intel.com>; shayd@nvidia.com
> Subject: Re: [iwl-next v2 03/15] ice: add basic devlink subfunctions support
> 
> On Mon, May 13, 2024 at 01:04:23PM +0200, Jiri Pirko wrote:
> > Mon, May 13, 2024 at 10:37:23AM CEST, michal.swiatkowski@linux.intel.com
> wrote:
> >
> > [...]
> >
> >
> >
> > >+int ice_devlink_create_sf_port(struct ice_dynamic_port *dyn_port)
> > >+{
> > >+	struct devlink_port_attrs attrs = {};
> > >+	struct devlink_port *devlink_port;
> > >+	struct devlink *devlink;
> > >+	struct ice_vsi *vsi;
> > >+	struct device *dev;
> > >+	struct ice_pf *pf;
> > >+	int err;
> > >+
> > >+	vsi = dyn_port->vsi;
> > >+	pf = dyn_port->pf;
> > >+	dev = ice_pf_to_dev(pf);
> > >+
> > >+	devlink_port = &dyn_port->devlink_port;
> > >+
> > >+	attrs.flavour = DEVLINK_PORT_FLAVOUR_PCI_SF;
> > >+	attrs.pci_sf.pf = pf->hw.bus.func;
> > >+	attrs.pci_sf.sf = dyn_port->sfnum;
> > >+
> > >+	devlink_port_attrs_set(devlink_port, &attrs);
> > >+	devlink = priv_to_devlink(pf);
> > >+
> > >+	err = devl_port_register_with_ops(devlink, devlink_port, vsi->idx,
> > >+					  &ice_devlink_port_sf_ops);
> > >+	if (err) {
> > >+		dev_err(dev, "Failed to create devlink port for Subfunction %d",
> > >+			vsi->idx);
> >
> > Either use extack or avoid this error message entirely. Could you please
> > double you don't write dmesg error messages in case you have extack
> > available in the rest of this patchset?
> >
> >
> 
> Sure, I can avoid, as this is called from port representor creeation
> function. I don't want to pass extack there (code is generic for VF and
> SF, and VF call doesn't have extack).

You can also pass an extack of NULL for flows which lack the extack, since all the extack functions are NULL-safe. Of course this does mean that you would end up with no error message logged in the VF case...

> 
> We have this pattern in few place in code (using dev_err even extack can
> be passed). Is it recommended to pass extact to all functions
> which probably want to write some message in case of error (assuming the
> call context has the extack)?
> 


Generally, yes. Extended ACK messages return and get logged on the command line of the application that issued the netlink message. This is significantly more visible than a log message from the driver.