linux-kernel - Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree

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:   Fri, 23 Sep 2022 12:03:14 +0300
From:   maukka@....kapsi.fi
To:     Andrew Lunn <andrew@...n.ch>
Cc:     robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        arnd@...db.de, olof@...om.net, sebastian.hesselbarth@...il.com,
        gregory.clement@...tlin.com, linux@...linux.org.uk,
        pali@...nel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device
 Tree

On 23.9.2022 00:10, Andrew Lunn wrote:
>> +static void __init dns323_dt_eth_fixup(void)
>> +{
>> +	struct device_node *np;
>> +	u8 addr[ETH_ALEN];
>> +	int ret;
>> +
>> +	/*
>> +	 * The ethernet interfaces forget the MAC address assigned by u-boot
>> +	 * if the clocks are turned off. Usually, u-boot on orion boards
>> +	 * has no DT support to properly set local-mac-address property.
>> +	 * As a workaround, we get the MAC address that is stored in flash
>> +	 * and update the port device node if no valid MAC address is set.
>> +	 */
> 
> This is true for Kirkwood, but orion5x does not have any clocks to
> gate. So i'm pretty sure this is not true. You copied this code for a
> different reason. Please document here the real reason for this code.
> 

Yes, will do. To my understanding it looks like uboot does not pass 
anything
to the kernel.

>> +	ret = dns323_read_mac_addr(addr);
>> +
>> +	if (ret) {
>> +		pr_warn("Unable to find MAC address in flash memory\n");
>> +		return;
>> +	}
>> +
>> +	np = of_find_compatible_node(NULL, NULL, "marvell,orion-eth-port");
>> +
>> +	if (!IS_ERR(np)) {
>> +		struct device_node *pnp = of_get_parent(np);
>> +		struct clk *clk;
>> +		struct property *pmac;
>> +		u8 tmpmac[ETH_ALEN];
>> +		u8 *macaddr;
>> +		int i;
>> +
>> +		if (!pnp)
>> +			return;
>> +
>> +		/* skip disabled nodes or nodes with valid MAC address*/
>> +		if (!of_device_is_available(pnp) ||
>> +		    !of_get_mac_address(np, tmpmac))
>> +			goto eth_fixup_skip;
>> +
>> +		clk = of_clk_get(pnp, 0);
>> +		if (IS_ERR(clk))
>> +			goto eth_fixup_skip;
>> +
>> +		/* ensure port clock is not gated to not hang CPU */
>> +		clk_prepare_enable(clk);
> 
> I'm pretty sure this clock stuff is not needed. Please comment it out
> and see if the machine locks up. Kirkwood just stops dead if you
> access registers when there clocks are disabled. For Orion5x, the
> ethernet should always have a clock.
> 

Will do.

>> +
>> +		/* store MAC address register contents in local-mac-address */
>> +		pmac = kzalloc(sizeof(*pmac) + 6, GFP_KERNEL);
>> +		if (!pmac)
>> +			goto eth_fixup_no_mem;
>> +
>> +		pmac->value = pmac + 1;
>> +		pmac->length = ETH_ALEN;
>> +		pmac->name = kstrdup("local-mac-address", GFP_KERNEL);
>> +		if (!pmac->name) {
>> +			kfree(pmac);
>> +			goto eth_fixup_no_mem;
>> +		}
>> +
>> +		macaddr = pmac->value;
>> +		for (i = 0; i < ETH_ALEN; i++)
>> +			macaddr[i] = addr[i];
>> +
>> +		of_update_property(np, pmac);
>> +
>> +eth_fixup_no_mem:
>> +		clk_disable_unprepare(clk);
>> +		clk_put(clk);
>> +eth_fixup_skip:
>> +		of_node_put(pnp);
>> +	}
>> +}
>> +
>> +void __init dns323_init_dt(void)
>> +{
>> +	if (of_machine_is_compatible("dlink,dns323a1")) {
>> +		writel(0, MPP_DEV_CTRL);		/* DEV_D[31:16] */
> 
> I spotted this in dns323-setup.c as well. Do you have any idea what it
> does?
> 

No idea. I have tried to replicate what was in dns323-setup.c as exactly 
as possible.
I can try to leave it out and see if anything changes.