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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 13 Apr 2023 18:12:28 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Ladislav Michl <oss-lists@...ops.cz>
Cc:     linux-staging@...ts.linux.dev, netdev@...r.kernel.org,
        linux-mips@...r.kernel.org,
        Chris Packham <chris.packham@...iedtelesis.co.nz>
Subject: Re: [PATCH 2/3] staging: octeon: avoid needless device allocation

>  	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.

> +		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.


>  			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.

	 Andrew