| 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: <f3a1ae88-2817-4c60-b281-336aedf18384@loongson.cn>
Date: Fri, 5 Apr 2024 19:13:51 +0800
From: Yanteng Si <siyanteng@...ngson.cn>
To: Serge Semin <fancer.lancer@...il.com>
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()
Hi Serge,
Sorry, I seem to have forgotten to reply to the comments on this patch.
在 2024/2/5 22:43, Serge Semin 写道:
> 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.
OK. I will resplit it.
>> 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?
OK, restore it.
>
>> +
>> + plat->mdio_bus_data->phy_mask = 0;
> This is already zero. Why do you need this?
OK, drop it.
>
>>
>> - /* Default to phy auto-detection */
> What is wrong with this comment?
Sorry, restore it.
>
>> 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.
OK.
>
>>
>> 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.
Sorry, restore it.
>
>> 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.
Hmm, I will merge it into [PATCH net-next 03/11] .
>
>> + 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
Yes, We only need to set phy_interface.
> 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.
OK.
>
>>
>> - 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
OK, I will.
Thanks,
Yanteng
Powered by blists - more mailing lists