| 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
| ||
|
Message-ID: <uvar72vvibm44tgn3trr52mpvrjgnn4ttbmyt2mouwws7pkywq@qcyrmj25c4su>
Date: Mon, 5 Feb 2024 17:43:42 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Yanteng Si <siyanteng@...ngson.cn>
Cc: andrew@...n.ch, hkallweit1@...il.com, peppe.cavallaro@...com,
alexandre.torgue@...s.st.com, joabreu@...opsys.com, Jose.Abreu@...opsys.com,
chenhuacai@...ngson.cn, linux@...linux.org.uk, guyinggang@...ngson.cn,
netdev@...r.kernel.org, chris.chenfeiyang@...il.com
Subject: Re: [PATCH net-next v8 02/11] net: stmmac: dwmac-loongson: Refactor
code for loongson_dwmac_probe()
On Tue, Jan 30, 2024 at 04:43:22PM +0800, Yanteng Si wrote:
> The driver function is not changed, but the code location is
> adjusted to prepare for adding more loongson drivers.
Having the word "refactoring" in the subject is always suspicious
because submitters very often try to hind behind it many small
changes they didn't want to/didn't know how to unpin from a more bulky
change. Moreover if there is no detailed explanation what is done and
why, it raises too many review questions and makes the reviewers life
much harder. So it would have been much better for us if you split up
this change into the smaller patches (see my last comment for a
presumable subset of the patches) to simplify the review process and
improve the driver bisectability especially seeing there actually are
functional changes introduced here despite of what is said in the
commit log.
>
> Signed-off-by: Yanteng Si <siyanteng@...ngson.cn>
> Signed-off-by: Feiyang Chen <chenfeiyang@...ngson.cn>
> Signed-off-by: Yinggang Gu <guyinggang@...ngson.cn>
> ---
> .../ethernet/stmicro/stmmac/dwmac-loongson.c | 61 +++++++++++++------
> 1 file changed, 42 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> index 9e40c28d453a..e2dcb339b8b0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> @@ -9,7 +9,12 @@
> #include <linux/of_irq.h>
> #include "stmmac.h"
>
> -static int loongson_default_data(struct plat_stmmacenet_data *plat)
> +struct stmmac_pci_info {
> + int (*setup)(struct pci_dev *pdev, struct plat_stmmacenet_data *plat);
> +};
> +
> +static void loongson_default_data(struct pci_dev *pdev,
> + struct plat_stmmacenet_data *plat)
> {
> plat->clk_csr = 2; /* clk_csr_i = 20-35MHz & MDC = clk_csr_i/16 */
> plat->has_gmac = 1;
> @@ -34,23 +39,37 @@ static int loongson_default_data(struct plat_stmmacenet_data *plat)
>
> /* Disable RX queues routing by default */
> plat->rx_queues_cfg[0].pkt_route = 0x0;
> +}
> +
> +static int loongson_gmac_data(struct pci_dev *pdev,
> + struct plat_stmmacenet_data *plat)
> +{
> + loongson_default_data(pdev, plat);
> +
> + plat->multicast_filter_bins = 256;
Why do you need to move this here from the function tail?
> +
> + plat->mdio_bus_data->phy_mask = 0;
This is already zero. Why do you need this?
>
> - /* Default to phy auto-detection */
What is wrong with this comment?
> plat->phy_addr = -1;
>
> plat->dma_cfg->pbl = 32;
> plat->dma_cfg->pblx8 = true;
>
> - plat->multicast_filter_bins = 256;
> return 0;
> }
>
> -static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +static struct stmmac_pci_info loongson_gmac_pci_info = {
> + .setup = loongson_gmac_data,
> +};
> +
> +static int loongson_dwmac_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> {
> + int ret, i, bus_id, phy_mode;
> struct plat_stmmacenet_data *plat;
> + struct stmmac_pci_info *info;
> struct stmmac_resources res;
> struct device_node *np;
> - int ret, i, phy_mode;
Reverse xmas tree order please.
>
> np = dev_of_node(&pdev->dev);
>
> @@ -69,18 +88,17 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
> if (!plat->mdio_bus_data)
> return -ENOMEM;
>
> + plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg),
> + GFP_KERNEL);
> + if (!plat->dma_cfg)
> + return -ENOMEM;
> +
Why do you need this moved above the mdio_node getting procedure? They
seem independent.
> plat->mdio_node = of_get_child_by_name(np, "mdio");
> if (plat->mdio_node) {
> dev_info(&pdev->dev, "Found MDIO subnode\n");
> plat->mdio_bus_data->needs_reset = true;
> }
>
> - plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL);
> - if (!plat->dma_cfg) {
> - ret = -ENOMEM;
> - goto err_put_node;
> - }
> -
> /* Enable pci device */
> ret = pci_enable_device(pdev);
> if (ret) {
> @@ -98,9 +116,16 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
> break;
> }
>
> - plat->bus_id = of_alias_get_id(np, "ethernet");
> - if (plat->bus_id < 0)
> - plat->bus_id = pci_dev_id(pdev);
This is a functional change because further bus_id is no longer
initialized by the pci_dev_id() method as a fallback case. If you are
sure this is required please unpin to a separate patch and explain.
> + pci_set_master(pdev);
> +
> + info = (struct stmmac_pci_info *)id->driver_data;
> + ret = info->setup(pdev, plat);
> + if (ret)
> + goto err_disable_device;
> +
> + bus_id = of_alias_get_id(np, "ethernet");
> + if (bus_id >= 0)
> + plat->bus_id = bus_id;
>
> phy_mode = device_get_phy_mode(&pdev->dev);
> if (phy_mode < 0) {
> @@ -110,11 +135,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
> }
>
> plat->phy_interface = phy_mode;
> - plat->mac_interface = PHY_INTERFACE_MODE_GMII;
This is just dropped. Are you sure that the driver will work correctly
after this change is applied? Russell already asked you about this change
here:
https://lore.kernel.org/netdev/ZZPnaziDZEcv5GGw@shell.armlinux.org.uk/
Anyway please unpin it to a separate patch and explain.
>
> - pci_set_master(pdev);
> -
> - loongson_default_data(plat);
> pci_enable_msi(pdev);
> memset(&res, 0, sizeof(res));
> res.addr = pcim_iomap_table(pdev)[0];
> @@ -212,8 +233,10 @@ static int __maybe_unused loongson_dwmac_resume(struct device *dev)
> static SIMPLE_DEV_PM_OPS(loongson_dwmac_pm_ops, loongson_dwmac_suspend,
> loongson_dwmac_resume);
>
> +#define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03
> +
> static const struct pci_device_id loongson_dwmac_id_table[] = {
> - { PCI_VDEVICE(LOONGSON, 0x7a03) },
> + { PCI_DEVICE_DATA(LOONGSON, GMAC, &loongson_gmac_pci_info) },
If I were you and needed to preserve all the changes I would have
split the patch up into the next patches:
1. Use PCI_DEVICE_DATA() macro for device identification
2. Drop mac-interface initialization
3. Don't initialize MDIO bus ID with PCIe device ID
4. Introduce device-specific setup callback
-Serge(y)
> {}
> };
> MODULE_DEVICE_TABLE(pci, loongson_dwmac_id_table);
> --
> 2.31.4
>
Powered by blists - more mailing lists