Re: [PATCH 2/3] staging: octeon: avoid needless device allocation

[Ladislav Michl <oss-lists@triops.cz>]

Hi Andrew,

On Thu, Apr 13, 2023 at 06:12:28PM +0200, Andrew Lunn wrote:
> >  	num_interfaces = cvmx_helper_get_number_of_interfaces();
> >  	for (interface = 0; interface < num_interfaces; interface++) {
> > +		int num_ports, port_index;
> > +		const struct net_device_ops *ops;
> > +		const char *name;
> > +		phy_interface_t phy_mode = PHY_INTERFACE_MODE_NA;
> >  		cvmx_helper_interface_mode_t imode =
> > -		    cvmx_helper_interface_get_mode(interface);
> > -		int num_ports = cvmx_helper_ports_on_interface(interface);
> > -		int port_index;
> > +			cvmx_helper_interface_get_mode(interface);
> > +
> > +		switch (imode) {
> > +		case CVMX_HELPER_INTERFACE_MODE_NPI:
> > +			ops = &cvm_oct_npi_netdev_ops;
> > +			name = "npi%d";
> 
> In general, the kernel does not give the interface names other than
> ethX. userspace can rename them, e.g. systemd with its persistent
> names. So as part of getting this driver out of staging, i would throw
> this naming code away.

That would break all userspace (which is often running vendor's kernel).
But since driver is in staging and noone cares about vendor's kernel
I guess it is okay...

> > +		num_ports = cvmx_helper_ports_on_interface(interface);
> >  		for (port_index = 0,
> >  		     port = cvmx_helper_get_ipd_port(interface, 0);
> >  		     port < cvmx_helper_get_ipd_port(interface, num_ports);
> >  		     port_index++, port++) {
> >  			struct octeon_ethernet *priv;
> >  			struct net_device *dev =
> > -			    alloc_etherdev(sizeof(struct octeon_ethernet));
> > +				alloc_etherdev(sizeof(struct octeon_ethernet));
> 
> Please try to avoid white space changed. Put such white space changes
> into a patch of their own, with a commit message saying it just
> contains whitespace cleanup.

Sorry, I overlooked this.

> >  			if (!dev) {
> >  				pr_err("Failed to allocate ethernet device for port %d\n",
> >  				       port);
> > @@ -830,7 +875,12 @@ static int cvm_oct_probe(struct platform_device *pdev)
> >  			priv->port = port;
> >  			priv->queue = cvmx_pko_get_base_queue(priv->port);
> >  			priv->fau = fau - cvmx_pko_get_num_queues(port) * 4;
> > -			priv->phy_mode = PHY_INTERFACE_MODE_NA;
> > +			priv->phy_mode = phy_mode;
> 
> You should be getting phy_mode from DT.
> 
> Ideally, you want lots of small patches which are obviously
> correct. So i would try to break this up into smaller changes.
> 
> I also wounder if you are addresses issues in the correct order. This
> driver is in staging for a reason. It needs a lot of work. You might
> be better off first cleaning it up. And then consider moving it to
> phylink.

I was asking this question myself and then came to this:
Converting driver to phylink makes separating different macs easier as
this driver is splitted between staging and arch/mips/cavium-octeon/executive/
However I'll provide changes spotted previously as separate preparational
patches. Would that work for you?

> 	 Andrew

Thank you,
	ladis