netdev - Re: [PATCH] [v3] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device

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, 24 Aug 2012 15:00:32 -0500
From:	Timur Tabi <timur@...escale.com>
To:	Stephen Warren <swarren@...dotorg.org>
CC:	Andy Fleming <afleming@...escale.com>,
	David Miller <davem@...emloft.net>, <ddaney.cavm@...il.com>,
	<devicetree-discuss@...ts.ozlabs.org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH] [v3] netdev/phy: add MDIO bus multiplexer driven by a
 memory-mapped device

Stephen Warren wrote:

>> +This is a special case of a MDIO bus multiplexer.  A memory-mapped device,
>> +like an FPGA, is used to control which child bus is connected.  The mdio-mux
>> +node must be a child of the memory-mapped device.  The driver currently only
>> +supports devices with eight-bit registers.
> 
> That last sentence seems like a property of the driver, not the binding;
> I could easily anticipate allowing the size to be 1 or 2 or 4, and a
> driver adapter to that in the future.

True, but I couldn't think of a better place to mention this.  Adding
support for multi-byte registers also requires handling the endianness of
those registers.  I have that problem with the mdio-mux-gpio driver.  That
driver assumes that the GPIO bits are numbered in little-endian order, so
my device tree on my big-endian CPU (PowerPC) lists the GPIO pins in
reverse order.

> Otherwise, this binding looks great now.

Do you still want me to scrub any references to the register size
requirement from the document?

>> +++ b/drivers/net/phy/mdio-mux-mmioreg.c
> 
>> +static int mdio_mux_mmioreg_switch_fn(int current_child, int desired_child,
>> +				      void *data)
>> +{
>> +	struct mdio_mux_mmioreg_state *s = data;
>> +
>> +	if (current_child ^ desired_child) {
>> +		void *p = ioremap(s->phys, 1);
>> +		uint8_t x, y;
>> +
>> +		if (!p)
>> +			return -ENOMEM;
> 
> Why not map it during probe?

I thought about that, but I generally don't like mappings that exist for
all eternity even though they're rarely used.  Once the interface is up,
we don't expect any bus muxing to occur.

> 
>> +		x = ioread8(p);
>> +		y = (x & ~s->mask) | desired_child;
>> +		if (x != y) {
> 
> Isn't that always true, given if (current_child ^ desired_child) above?

If current_child == -1, but the bus is already muxed properly, then
there's no point in setting it.  Do you want me to remove the test, or add
a comment?

-- 
Timur Tabi
Linux kernel developer at Freescale

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html