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
| ||
|
Date: Thu, 4 Apr 2013 14:51:51 +0530 From: Kishon Vijay Abraham I <kishon@...com> To: Sylwester Nawrocki <sylvester.nawrocki@...il.com> CC: <balbi@...com>, <gregkh@...uxfoundation.org>, <arnd@...db.de>, <akpm@...ux-foundation.org>, <swarren@...dotorg.org>, <rob@...dley.net>, <netdev@...r.kernel.org>, <davem@...emloft.net>, <cesarb@...arb.net>, <linux-usb@...r.kernel.org>, <linux-omap@...r.kernel.org>, <linux-kernel@...r.kernel.org>, <tony@...mide.com>, <grant.likely@...retlab.ca>, <rob.herring@...xeda.com>, <b-cousson@...com>, <linux@....linux.org.uk>, <eballetbo@...il.com>, <javier@...hile0.org>, <mchehab@...hat.com>, <santosh.shilimkar@...com>, <broonie@...nsource.wolfsonmicro.com>, <swarren@...dia.com>, <linux-doc@...r.kernel.org>, <devicetree-discuss@...ts.ozlabs.org>, <linux-arm-kernel@...ts.infradead.org> Subject: Re: [PATCH v5 1/6] drivers: phy: add generic PHY framework Hi, On Thursday 04 April 2013 03:16 AM, Sylwester Nawrocki wrote: > On 04/03/2013 02:53 PM, Kishon Vijay Abraham I wrote: . . <snip> . . >> diff --git a/Documentation/phy.txt b/Documentation/phy.txt >> new file mode 100644 >> index 0000000..7785ec0 >> --- /dev/null >> +++ b/Documentation/phy.txt >> @@ -0,0 +1,113 @@ >> + PHY SUBSYSTEM >> + Kishon Vijay Abraham I<kishon@...com> >> + >> +This document explains the Generic PHY Framework along with the APIs >> provided, >> +and how-to-use. >> + >> +1. Introduction >> + >> +*PHY* is the abbreviation for physical layer. It is used to connect a >> device >> +to the physical medium e.g., the USB controller has a PHY to provide >> functions > > Shouldn't it be "...medium, e.g. the USB..." ? > >> +such as serialization, de-serialization, encoding, decoding and is >> responsible >> +for obtaining the required data transmission rate. Note that some USB >> +controller has PHY functionality embedded into it and others use an >> external > > "controllers have PHY functionality embedded into them" ? > >> +PHY. Other peripherals that uses a PHY include Wireless LAN, Ethernet, > > s/uses/use ? > >> +SATA etc. >> + >> +The intention of creating this framework is to bring the phy drivers >> spread >> +all over the Linux kernel to drivers/phy to increase code re-use and to >> +increase code maintainability. >> + >> +This framework will be of use only to devices that uses external PHY >> (PHY > > s/uses/use ? > >> +functionality is not embedded within the controller). >> + >> +2. Creating the PHY >> + >> +The PHY driver should create the PHY in order for other peripheral >> controllers >> +to make use of it. The PHY framework provides 2 APIs to create the PHY. >> + >> +struct phy *phy_create(struct device *dev, const char *label, >> + struct device_node *of_node, int type, struct phy_ops *ops, >> + void *priv); >> +struct phy *devm_phy_create(struct device *dev, const char *label, >> + struct device_node *of_node, int type, struct phy_ops *ops, >> + void *priv); >> + >> +The PHY drivers can use one of the above 2 APIs to create the PHY by >> passing >> +the device pointer, label, device node, type, phy ops and a driver data. >> +phy_ops is a set of function pointers for performing PHY operations >> such as >> +init, exit, suspend, resume, power_on and power_off. >> + >> +3. Binding the PHY to the controller >> + >> +The framework provides an API for binding the controller to the PHY >> in the >> +case of non dt boot. >> + >> +struct phy_bind *phy_bind(const char *dev_name, int index, >> + const char *phy_dev_name); >> + >> +The API fills the phy_bind structure with the dev_name (device name >> of the >> +controller), index and phy_dev_name (device name of the PHY). This will >> +be used when the controller requests this phy. This API should be >> used by >> +platform specific initialization code (board file). >> + >> +In the case of dt boot, the binding information should be added in >> the dt >> +data of the controller. > > s/in the dt data of/in the device tree node of ? > >> +4. Getting a reference to the PHY >> + >> +Before the controller can make use of the PHY, it has to get a >> reference to >> +it. This framework provides 6 APIs to get a reference to the PHY. >> + >> +struct phy *phy_get(struct device *dev, int index); >> +struct phy *devm_phy_get(struct device *dev, int index); >> +struct phy *of_phy_get(struct device *dev, const char *phandle, int >> index); >> +struct phy *devm_of_phy_get(struct device *dev, const char *phandle, >> int index); > > Why do we need separate functions for dt and non-dt ? Couldn't it be > handled > internally by the framework ? So the PHY users can use same calls for dt > and non-dt, like in case of e.g. the regulators API ? yeah. Indeed it makes sense to do it that way. > > Also signatures of some functions are different now: > > extern struct phy *phy_get(struct device *dev, int index); > extern struct phy *devm_phy_get(struct device *dev, int index); > extern struct phy *of_phy_get(struct device *dev, int index); > extern struct phy *devm_of_phy_get(struct device *dev, int index); My bad :-( > > BTW, I think "extern" could be dropped, does it have any significance in > function declarations in header files ? it shouldn't have any effect I guess. It's harmless nevertheless. > >> +struct phy *of_phy_get_byname(struct device *dev, const char *string); >> +struct phy *devm_of_phy_get_byname(struct device *dev, const char >> *string); >> + >> +phy_get and devm_phy_get can be used to get the PHY in non-dt boot. >> This API >> +uses the binding information added using the phy_bind API to find and >> return >> +the appropriate PHY. The only difference between the two APIs is that >> +devm_phy_get associates the device with the PHY using devres on >> successful PHY >> +get. On driver detach, release function is invoked on the the devres >> data and > > s/the the/the > >> +devres data is freed. >> + >> +of_phy_get and devm_of_phy_get can be used to get the PHY in dt boot. >> These >> +APIs take the phandle and index to get a reference to the PHY. The only >> +difference between the two APIs is that devm_of_phy_get associates >> the device >> +with the PHY using devres on successful phy get. On driver detach, >> release >> +function is invoked on the devres data and it is freed. >> + >> +of_phy_get_byname and devm_of_phy_get_byname can also be used to get >> the PHY >> +in dt boot. It is same as the above API except that the user has to >> pass the >> +phy name as filled in "phy-names" phandle in dt data and the >> framework will > > s/phandle/property ? > >> +find the index and get the PHY. >> + >> +5. Releasing a reference to the PHY >> + >> +When the controller no longer needs the PHY, it has to release the >> reference >> +to the PHY it has obtained using the APIs mentioned in the above >> section. The >> +PHY framework provides 2 APIS to release a reference to the PHY. > > s/APIS/APIs ? > >> + >> +void phy_put(struct phy *phy); >> +void devm_phy_put(struct device *dev, struct phy *phy); >> + >> +Both these APIs are used to release a reference to the PHY and >> devm_phy_put >> +destroys the devres associated with this PHY. >> + >> +6. Destroying the PHY >> + >> +When the driver that created the PHY is unloaded, it should destroy >> the PHY it >> +created using one of the following 2 APIs. >> + >> +void phy_destroy(struct phy *phy); >> +void devm_phy_destroy(struct device *dev, struct phy *phy); >> + >> +Both these APIs destroys the PHY and devm_phy_destroy destroys the >> devres > > s/APIs destroys/APIs destroy ? > >> +associated with this PHY. >> + >> +7. DeviceTree Binding >> + >> +The documentation for PHY dt binding can be found @ >> +Documentation/devicetree/bindings/phy/phy-bindings.txt > >> --- /dev/null >> +++ b/drivers/phy/phy-core.c >> @@ -0,0 +1,616 @@ > >> +static struct phy *of_phy_lookup(struct device_node *node) >> +{ >> + struct phy *phy; >> + struct device *dev; >> + struct class_dev_iter iter; >> + >> + class_dev_iter_init(&iter, phy_class, NULL, NULL); > > There is currently nothing preventing a call to this function before > phy_class is initialized. It happened during my tests, and the nice > stack dump showed that it was the PHY user attempting to get the PHY > before the framework got initialized. So either phy_core_init should > be a subsys_initcall or we need a better protection against phy_class > being NULL or ERR_PTR in more places. Whats the initcall used in your PHY user? Looks like more people prefer having module_init and any issue because of it should be fixed in PHY providers and PHY users. > >> + while ((dev = class_dev_iter_next(&iter))) { >> + phy = container_of(dev, struct phy, dev); >> + if (node != phy->of_node) >> + continue; >> + >> + class_dev_iter_exit(&iter); >> + return phy; >> + } >> + >> + class_dev_iter_exit(&iter); >> + return ERR_PTR(-EPROBE_DEFER); >> +} > >> +/** >> + * of_phy_get() - lookup and obtain a reference to a phy by phandle >> + * @dev: device that requests this phy >> + * @index: the index of the phy >> + * >> + * Returns the phy associated with the given phandle value, >> + * after getting a refcount to it or -ENODEV if there is no such phy or >> + * -EPROBE_DEFER if there is a phandle to the phy, but the device is >> + * not yet loaded. >> + */ >> +struct phy *of_phy_get(struct device *dev, int index) >> +{ >> + int ret; >> + struct phy *phy = NULL; >> + struct phy_bind *phy_map = NULL; >> + struct of_phandle_args args; >> + struct device_node *node; >> + >> + if (!dev->of_node) { >> + dev_dbg(dev, "device does not have a device node entry\n"); >> + return ERR_PTR(-EINVAL); >> + } >> + >> + ret = of_parse_phandle_with_args(dev->of_node, "phys", "#phy-cells", >> + index,&args); >> + if (ret) { >> + dev_dbg(dev, "failed to get phy in %s node\n", >> + dev->of_node->full_name); >> + return ERR_PTR(-ENODEV); >> + } >> + >> + phy = of_phy_lookup(args.np); >> + if (IS_ERR(phy) || !try_module_get(phy->ops->owner)) { >> + phy = ERR_PTR(-EPROBE_DEFER); >> + goto err0; >> + } >> + >> + phy = phy->ops->of_xlate(phy,&args); >> + if (IS_ERR(phy)) >> + goto err1; >> + >> + phy_map = phy_bind(dev_name(dev), index, dev_name(&phy->dev)); >> + if (!IS_ERR(phy_map)) { >> + phy_map->phy = phy; >> + phy_map->auto_bind = true; > > Shouldn't it be done with the phy_bind_mutex held ? I guess an unlocked > version of the phy_bind functions is needed, so it can be used internally. The locking is done inside phy_bind function. I'm not sure but I vaguely remember getting a dump stack (incorrect mutex ordering or something) when trying to have phy_bind_mutex here. I can check it again. > >> + } >> + >> + get_device(&phy->dev); >> + >> +err1: >> + module_put(phy->ops->owner); >> + >> +err0: >> + of_node_put(node); >> + >> + return phy; >> +} >> +EXPORT_SYMBOL_GPL(of_phy_get); > >> +/** >> + * phy_create() - create a new phy >> + * @dev: device that is creating the new phy >> + * @label: label given to phy >> + * @of_node: device node of the phy >> + * @type: specifies the phy type >> + * @ops: function pointers for performing phy operations >> + * @priv: private data from phy driver >> + * >> + * Called to create a phy using phy framework. >> + */ >> +struct phy *phy_create(struct device *dev, const char *label, >> + struct device_node *of_node, int type, struct phy_ops *ops, >> + void *priv) >> +{ >> + int ret; >> + struct phy *phy; >> + struct phy_bind *phy_bind; >> + const char *devname = NULL; >> + >> + if (!dev) { >> + dev_err(dev, "no device provided for PHY\n"); >> + ret = -EINVAL; >> + goto err0; >> + } >> + >> + if (!ops || !ops->of_xlate || !priv) { >> + dev_err(dev, "no PHY ops/PHY data provided\n"); >> + ret = -EINVAL; >> + goto err0; >> + } >> + >> + if (!phy_class) >> + phy_core_init(); >> + >> + phy = kzalloc(sizeof(*phy), GFP_KERNEL); >> + if (!phy) { >> + ret = -ENOMEM; >> + goto err0; >> + } >> + >> + devname = dev_name(dev); > > OK, a sort of serious issue here is that you can't call phy_create() > multiple times for same PHY provider device. Ah.. right. > > device_add() further will fail as sysfs won't let you create multiple > objects with same name. So I would assume we need to add a new argument > to phy_create() or rename @type to e.g. @phy_id, which could be > concatenated with dev_name(dev) to create a unique name of the phy > device. hmm.. ok > > And how is the PHY provider supposed to identify a PHY in its common PHY > ops, now when the struct phy port field is removed ? I have temporarily > (ab)used the type field in order to select proper registers within the > phy ops. Can't it be handled in the PHY provider driver without using struct phy *? Moreover the PHY ops passes on the *phy to phy provider right? Not sure I understand you here. > >> + device_initialize(&phy->dev); >> + >> + phy->dev.class = phy_class; >> + phy->dev.parent = dev; >> + phy->label = label; > > What about duplicating the string here ? That would make the API a bit > more convenient and the callers wouldn't be required to keep all the > labels around. you mean use dev_name? The device names are sometime more cryptic so preferred to have it like this. > >> + phy->of_node = of_node; >> + phy->type = type; >> + phy->ops = ops; >> + >> + dev_set_drvdata(&phy->dev, priv); >> + >> + ret = dev_set_name(&phy->dev, "%s", devname); >> + if (ret) >> + goto err1; >> + >> + mutex_lock(&phy_bind_mutex); >> + list_for_each_entry(phy_bind,&phy_bind_list, list) >> + if (!(strcmp(phy_bind->phy_dev_name, devname))) >> + phy_bind->phy = phy; >> + mutex_unlock(&phy_bind_mutex); >> + >> + ret = device_add(&phy->dev); >> + if (ret) >> + goto err2; >> + >> + return phy; >> + >> +err2: >> + phy_bind->phy = NULL; >> + >> +err1: >> + put_device(&phy->dev); >> + kfree(phy); >> + >> +err0: >> + return ERR_PTR(ret); >> +} >> +EXPORT_SYMBOL_GPL(phy_create); > >> +/** >> + * devm_phy_create() - create a new phy >> + * @dev: device that is creating the new phy >> + * @dev: device that is creating the new phy > > Duplicated line. > >> + * @label: label given to phy >> + * @of_node: device node of the phy >> + * @type: specifies the phy type >> + * @ops: function pointers for performing phy operations >> + * @priv: private data from phy driver >> + * >> + * Creates a new PHY device adding it to the PHY class. >> + * While at that, it also associates the device with the phy using >> devres. >> + * On driver detach, release function is invoked on the devres data, >> + * then, devres data is freed. >> + */ > >> +/** >> + * phy_bind() - bind the phy and the controller that uses the phy >> + * @dev_name: the device name of the device that will bind to the phy >> + * @index: index to specify the port number >> + * @phy_dev_name: the device name of the phy >> + * >> + * Fills the phy_bind structure with the dev_name and phy_dev_name. >> This will >> + * be used when the phy driver registers the phy and when the controller >> + * requests this phy. >> + * >> + * To be used by platform specific initialization code. >> + */ >> +struct phy_bind *phy_bind(const char *dev_name, int index, >> + const char *phy_dev_name) >> +{ >> + struct phy_bind *phy_bind; >> + >> + mutex_lock(&phy_bind_mutex); >> + list_for_each_entry(phy_bind,&phy_bind_list, list) { >> + if (!strcmp(phy_bind->dev_name, dev_name)&& phy_bind->index == >> + index) { >> + phy_bind->phy_dev_name = phy_dev_name; >> + goto ret0; >> + } >> + } >> + >> + phy_bind = kzalloc(sizeof(*phy_bind), GFP_KERNEL); >> + if (!phy_bind) { >> + phy_bind = ERR_PTR(-ENOMEM); >> + goto ret0; >> + } >> + >> + phy_bind->dev_name = dev_name; >> + phy_bind->phy_dev_name = phy_dev_name; >> + phy_bind->index = index; >> + phy_bind->auto_bind = false; >> + >> + list_add_tail(&phy_bind->list,&phy_bind_list); >> + >> +ret0: >> + mutex_unlock(&phy_bind_mutex); >> + return phy_bind; >> +} >> +EXPORT_SYMBOL_GPL(phy_bind); > >> +static ssize_t phy_bind_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct phy_bind *phy_bind; >> + struct phy *phy; >> + char *p = buf; >> + int len; >> + >> + phy = container_of(dev, struct phy, dev); >> + > > Shouldn't this iteration also be protected with the phy_bind_mutex ? Indeed. > >> + list_for_each_entry(phy_bind,&phy_bind_list, list) >> + if (phy_bind->phy == phy) >> + p += sprintf(p, "%s %d %s\n", phy_bind->dev_name, >> + phy_bind->index, phy_bind->phy_dev_name); >> + len = (p - buf); >> + >> + return len; >> +} > >> +static int phy_core_init(void) >> +{ >> + if (phy_class) >> + return 0; >> + >> + phy_class = class_create(THIS_MODULE, "phy"); >> + if (IS_ERR(phy_class)) { >> + pr_err("failed to create phy class --> %ld\n", >> + PTR_ERR(phy_class)); >> + return PTR_ERR(phy_class); >> + } >> + >> + phy_class->dev_release = phy_release; >> + phy_class->dev_attrs = phy_dev_attrs; >> + >> + return 0; >> +} >> +module_init(phy_core_init); > > Having this framework initialized before the PHY provider and consumer > drivers could save some unnecessary probe deferrals. I was wondering, > what is really an advantage of having it as module_init(), rather than > subsys_initcall() ? I'm not sure of the exact reason but after the advent of EPROBE_DEFER, everyone recommends to use module_init only. Thanks for the detailed review. Regards Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists