linux-kernel - Re: [PATCH v2 net-next 3/6] net: bcmasp: Add support for ASP2.0 Ethernet controller

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]

Message-ID: <b1ee9be3-60db-31ea-97dd-916dc80f237c@wanadoo.fr>
Date:   Tue, 2 May 2023 22:24:08 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Justin Chen <justinpopo6@...il.com>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-media@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        bcm-kernel-feedback-list@...adcom.com
Cc:     justin.chen@...adcom.com, f.fainelli@...il.com,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, opendmb@...il.com,
        andrew@...n.ch, hkallweit1@...il.com, linux@...linux.org.uk,
        richardcochran@...il.com, sumit.semwal@...aro.org,
        christian.koenig@....com
Subject: Re: [PATCH v2 net-next 3/6] net: bcmasp: Add support for ASP2.0
 Ethernet controller

Le 26/04/2023 à 20:54, Justin Chen a écrit :
> Add support for the Broadcom ASP 2.0 Ethernet controller which is first
> introduced with 72165. This controller features two distinct Ethernet
> ports that can be independently operated.
> 
> This patch supports:
> 
> - Wake-on-LAN using magic packets
> - basic ethtool operations (link, counters, message level)
> - MAC destination address filtering (promiscuous, ALL_MULTI, etc.)
> 
> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
> Signed-off-by: Justin Chen <justinpopo6@...il.com>
> ---

[...]

> +void bcmasp_disable_all_filters(struct bcmasp_intf *intf)
> +{
> +	struct bcmasp_priv *priv = intf->parent;
> +	unsigned int i;

Hi,

Nit: Some loop index are unsigned int, but most are int.
This could be done consistantly.

> +
> +	/* Disable all filters held by this port */
> +	for (i = ASP_RX_FILT_MDA_RES_COUNT(intf); i < NUM_MDA_FILTERS; i++) {
> +		if (priv->mda_filters[i].en &&
> +		    priv->mda_filters[i].port == intf->port)
> +			bcmasp_en_mda_filter(intf, 0, i);
> +	}
> +}

[...]

> +static int bcmasp_probe(struct platform_device *pdev)
> +{
> +	struct device_node *ports_node, *intf_node;
> +	const struct bcmasp_plat_data *pdata;
> +	struct device *dev = &pdev->dev;
> +	int ret, i, count = 0, port;
> +	struct bcmasp_priv *priv;
> +	struct bcmasp_intf *intf;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->irq = platform_get_irq(pdev, 0);
> +	if (priv->irq <= 0) {
> +		dev_err(dev, "invalid interrupt\n");
> +		return -EINVAL;
> +	}
> +
> +	priv->clk = devm_clk_get_optional_enabled(dev, "sw_asp");
> +	if (IS_ERR(priv->clk)) {
> +		dev_err(dev, "failed to request clock\n");
> +		return PTR_ERR(priv->clk);
> +	}
> +
> +	/* Base from parent node */
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base)) {
> +		dev_err(dev, "failed to iomap\n");
> +		return PTR_ERR(priv->base);
> +	}
> +
> +	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
> +	if (ret)
> +		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));

I don't think that this fallback is needed.
See [1].

More over, using dev_err_probe() would slighly simplify the probe 
function. (saves a few LoC, logs the error code in a human reading format)

[1]: 
https://lore.kernel.org/lkml/86bf852e-4220-52d4-259d-3455bc24def1@wanadoo.fr/T/#m022abc0051ede3ba1feeb06cefd59e2a8a5c7864

> +	if (ret) {
> +		dev_err(&pdev->dev, "unable to set DMA mask: %d\n", ret);
> +		return ret;
> +	}
> +

[...]

> +static int __maybe_unused bcmasp_suspend(struct device *d)
> +{
> +	struct bcmasp_priv *priv = dev_get_drvdata(d);
> +	struct bcmasp_intf *intf;
> +	unsigned int i;

Same

> +	int ret = 0;

no need to initialize, but it is mostmy a matter of taste.

> +
> +	for (i = 0; i < priv->intf_count; i++) {
> +		intf = priv->intfs[i];
> +		if (!intf)
> +			continue;
> +
> +		ret = bcmasp_interface_suspend(intf);
> +		if (ret)
> +			break;
> +	}
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		return ret;
> +
> +	/* Whether Wake-on-LAN is enabled or not, we can always disable
> +	 * the shared TX clock
> +	 */
> +	bcmasp_core_clock_set(priv, 0, ASP_CTRL_CLOCK_CTRL_ASP_TX_DISABLE);
> +
> +	bcmasp_core_clock_select(priv, true);
> +
> +	clk_disable_unprepare(priv->clk);
> +
> +	return ret;
> +}
> +
> +static int __maybe_unused bcmasp_resume(struct device *d)
> +{
> +	struct bcmasp_priv *priv = dev_get_drvdata(d);
> +	struct bcmasp_intf *intf;
> +	unsigned int i;

same

> +	int ret = 0;

no need to initialize, but it is mostmy a matter of taste.

Just my 2c,
CJ