netdev - Re: [PATCH v2 1/5] drivers: phy: add generic PHY framework

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: <201302191428.42407.arnd@arndb.de>
Date:	Tue, 19 Feb 2013 14:28:42 +0000
From:	Arnd Bergmann <arnd@...db.de>
To:	kishon <kishon@...com>
Cc:	rob@...dley.net, tony@...mide.com, linux@....linux.org.uk,
	eballetbo@...il.com, javier@...hile0.org, balbi@...com,
	gregkh@...uxfoundation.org, akpm@...ux-foundation.org,
	mchehab@...hat.com, cesarb@...arb.net, davem@...emloft.net,
	santosh.shilimkar@...com, broonie@...nsource.wolfsonmicro.com,
	swarren@...dia.com, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-omap@...r.kernel.org, linux-usb@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH v2 1/5] drivers: phy: add generic PHY framework

On Tuesday 19 February 2013, kishon wrote:
> >> +
> >> +	devname = dev_name(dev);
> >> +	device_initialize(&phy->dev);
> >> +	phy->desc = desc;
> >> +	phy->dev.class = phy_class;
> >> +	phy->dev.parent = dev;
> >> +	phy->dev.bus = desc->bus;
> >> +	ret = dev_set_name(&phy->dev, "%s", devname);
> >
> >
> > Passing a bus_type through the descriptor seems misplaced. What is this for?
> 
> I thought if we are adding ethernet phys here (say drivers/phy/net), we 
> can make phy_device_create() (currently in drivers/net/phy/phy_device.c) 
> call phy_create with bus_type set to mdio_bus_type. Then we can have all 
> the PHYs showing up in /sys/class/phy/ and ethernet can continue to use 
> its own phy abstraction layer.

Hmm, that relies on the fact that mdio uses a 'bus_type' while the new phy
support uses a 'class', and it will break if we ever get to the point
where those two concepts are merged. I would rather not plan ahead here.

> > Why is this function not just:
> >
> > struct phy *phy_create(struct device *dev, const char *label, int type,
> > 			struct phy_ops *ops);
> 
> since while calling the callback functions using ops, there wont be 
> anyway to get back the device specific structure pointer.
> 
> struct phy_dev {
>   	.
>          .
> 	struct phy_descriptor	desc;
> 	void __iomem		*base;
> 	.
> 	.
> };
> 
> static int phy_resume(struct phy_descriptor *desc)
> {
> 
> //if we dont pass a member of phy_dev while *phy_create* we can't get 
> back phy_dev from callback functions as used below.
> 	struct phy_dev *phy = desc_to_omapusb(desc);
> 
> 	return 0;
> }
> 
> static struct phy_ops ops = {
> 	.resume		= phy_resume,
> 	.owner		= THIS_MODULE,
> };


In other subsystems, that is what the device->private_data pointer is used
for, which you could also pass to phy_create, or set after calling that
function.

> > Passing a structure like you do here seems dangerous because when someone
> > decides to add members to the structure, existing code will not give a
> > build error but silently break.
> 
> Not sure I understood this point. Care to explain?

Nevermind, when I wrote that sentence, I had not yet noticed that the
phy_descriptor is kept around. I was thinking that the structure was
only used to pass more arguments into phy_create.

	Arnd
--
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