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
| ||
|
Message-ID: <20230405194353.pwuk7e6rxnha3uqi@pengutronix.de> Date: Wed, 5 Apr 2023 21:43:53 +0200 From: Marco Felsch <m.felsch@...gutronix.de> To: Andrew Lunn <andrew@...n.ch> Cc: Heiner Kallweit <hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Florian Fainelli <f.fainelli@...il.com>, Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>, Richard Cochran <richardcochran@...il.com>, Radu Pirea <radu-nicolae.pirea@....nxp.com>, Shyam Sundar S K <Shyam-sundar.S-k@....com>, Yisen Zhuang <yisen.zhuang@...wei.com>, Salil Mehta <salil.mehta@...wei.com>, Jassi Brar <jaswinder.singh@...aro.org>, Ilias Apalodimas <ilias.apalodimas@...aro.org>, Iyappan Subramanian <iyappan@...amperecomputing.com>, Keyur Chudgar <keyur@...amperecomputing.com>, Quan Nguyen <quan@...amperecomputing.com>, "Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>, Rob Herring <robh+dt@...nel.org>, Frank Rowand <frowand.list@...il.com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org, devicetree@...r.kernel.org, kernel@...gutronix.de Subject: Re: [PATCH 06/12] net: phy: add phy_device_atomic_register helper On 23-04-05, Andrew Lunn wrote: > > The current fwnode_mdio.c don't provide the proper helper functions yet. > > Instead the parsing is spread between fwnode_mdiobus_register_phy() and > > fwnode_mdiobus_phy_device_register(). Of course these can be extracted > > and exported but I don't see the benefit. IMHO it just cause jumping > > around files and since fwnode is a proper firmware abstraction we could > > use is directly wihin core/lib files. > > No, assuming fwnode is the proper firmware abstraction is wrong. You > need to be very careful any time you convert of_ to fwnode_ and look > at the history of every property. Look at the number of deprecated OF > properties in Documentation/devicetree/bindings. They should never be > moved to fwnode_ because then you are moving deprecated properties to > ACPI, which never had them in the first place! The handling of deprecated properties is always a pain. Drivers handling deprecated properties correctly for of_ should handle it correctly for fwnode_ too. IMHO it would be driver bug if not existing deprecated properties cause an error. Of course there will be properties which need special attention for ACPI case but I don't see a problem for deprecated properties since those must be handled correctly for of_ case too. > You cannot assume DT and ACPI are the same thing, have the same > binding. And the same is true, in theory, in the opposite direction. > We don't want the DT properties polluted with ACPI only properties. > Not that anybody takes ACPI seriously in networking. My assumption was that ACPI is becoming more closer to OF and the fwnode/device abstraction is the abstraction to have one driver interacting correctly with OF and ACPI. As I said above there will be some corner-cases which need special attention of course :/ Also while answering this mail, I noticed that there are already some 'small' fwnode/device_ helpers within phy_device.c. So why not bundling everything within phy_device.c? > > I know and I thought about adding the firmware parsing helpers first but > > than I went this way. I can split this of course to make the patch > > smaller. > > Please do. Also, i read your commit message thinking it was a straight > copy of the code, and hence i did not need to review the code. But in > fact it is new code. So i need to take a close look at it. > > But what i think is most important for this patchset is the > justification for not fixing the current API. Why is it broken beyond > repair? Currently we have one API which creates/allocates the 'struct phy_device' and intialize the state which is: - phy_device_create() This function requests a driver based on the phy_id/c45_ids. The ID have to come from somewhere if autodection is used. For autodetection case - get_phy_device() is called. This function try to access the phy without taken possible hardware dependencies into account. These dependecies can be reset-lines (in my case), clocks, supplies, ... For taking fwnode (and possible dependencies) into account fwnode_mdio.c was written which provides two helpers: - fwnode_mdiobus_register_phy() - fwnode_mdiobus_phy_device_register(). The of_mdio.c and of_mdiobus_register_phy() is just a wrapper around fwnode_mdiobus_register_phy(). fwnode_mdiobus_register_phy(): 1st) calls get_phy_device() in case of autodection or c45. If phy_id is provided and !c45 case phy_device_create() is called to get a 'struct phy_device' - The autodection/c45 case try to access the PHYID registers which is not possible, please see above. 2nd) call fwnode_mdiobus_phy_device_register() or phy_device_register() directly. - phy_device_register() is the first time we taking the possible hardware reset line into account, which is far to late. fwnode_mdiobus_phy_device_register(): - takes a 'struct phy_device' as parameter, again this have to come from somewhere. - calls phy_device_register() which is taken the possibel hardware reset into account, again to late. Why do I need the autodection? Because PHYs can be changed due to EOL, cheaper device, ... I don't wanna have a new devicetree/firmware for the updated product, just let the magic happen :) Why do I introduce a new API? 1st) There are working users of get_phy_device() and I don't wanna break their systems, so I left this logic untouched. 2nd) The fwnode API is replaced by this new one, since it is broken (please see above). 3rd) IMHO the 'phy request/phy create' handling is far to complex therefore I introduced a single API which: - intialize all structures and states - prepare the device for interaction by using fwnode - initialize/detect the device and requests the coorect phy module - applies the fixups - add the device to the kernel - finally return the 'struct phy_device' to the user, so the driver can do $stuff. 4th) The new 'struct phy_device_config' makes it easier to adapt/extend the API. Thanks a lot for your fast response and feedback :) Regards, Marco
Powered by blists - more mailing lists