| Message ID | 20210209164051.18156-8-michael@walle.cc (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: icplus: cleanups and new features | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 92 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, Feb 09, 2021 at 05:40:49PM +0100, Michael Walle wrote: > Registers >= 16 are paged. Be sure to set the page. It seems this was > working for now, because the default is correct for the registers used > in the driver at the moment. But this will also assume, nobody will > change the page select register before linux is started. The page select > register is _not_ reset with a soft reset of the PHY. > > Add read_page()/write_page() support for the IP101G and use it > accordingly. > > Signed-off-by: Michael Walle <michael@walle.cc> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On 09.02.2021 17:40, Michael Walle wrote: > Registers >= 16 are paged. Be sure to set the page. It seems this was > working for now, because the default is correct for the registers used > in the driver at the moment. But this will also assume, nobody will > change the page select register before linux is started. The page select > register is _not_ reset with a soft reset of the PHY. > > Add read_page()/write_page() support for the IP101G and use it > accordingly. > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > drivers/net/phy/icplus.c | 50 +++++++++++++++++++++++++++++++--------- > 1 file changed, 39 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c > index a6e1c7611f15..858b9326a72d 100644 > --- a/drivers/net/phy/icplus.c > +++ b/drivers/net/phy/icplus.c > @@ -49,6 +49,8 @@ MODULE_LICENSE("GPL"); > #define IP101G_DIGITAL_IO_SPEC_CTRL 0x1d > #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32 BIT(2) > > +#define IP101G_DEFAULT_PAGE 16 > + > #define IP175C_PHY_ID 0x02430d80 > #define IP1001_PHY_ID 0x02430d90 > #define IP101A_PHY_ID 0x02430c54 > @@ -250,23 +252,25 @@ static int ip101a_g_probe(struct phy_device *phydev) > static int ip101a_g_config_init(struct phy_device *phydev) > { > struct ip101a_g_phy_priv *priv = phydev->priv; > - int err; > + int oldpage, err; > + > + oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE); > > /* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: */ > switch (priv->sel_intr32) { > case IP101GR_SEL_INTR32_RXER: > - err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL, > - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0); > + err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL, > + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0); > if (err < 0) > - return err; > + goto out; > break; > > case IP101GR_SEL_INTR32_INTR: > - err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL, > - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, > - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32); > + err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL, > + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, > + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32); > if (err < 0) > - return err; > + goto out; > break; > > default: > @@ -284,12 +288,14 @@ static int ip101a_g_config_init(struct phy_device *phydev) > * reserved as 'write-one'. > */ > if (priv->model == IP101A) { > - err = phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS, IP101A_G_APS_ON); > + err = __phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS, > + IP101A_G_APS_ON); > if (err) > - return err; > + goto out; > } > > - return 0; > +out: > + return phy_restore_page(phydev, oldpage, err); If a random page was set before entering config_init, do we actually want to restore it? Or wouldn't it be better to set the default page as part of initialization? > } > > static int ip101a_g_ack_interrupt(struct phy_device *phydev) > @@ -347,6 +353,26 @@ static irqreturn_t ip101a_g_handle_interrupt(struct phy_device *phydev) > return IRQ_HANDLED; > } > > +static int ip101a_g_read_page(struct phy_device *phydev) > +{ > + struct ip101a_g_phy_priv *priv = phydev->priv; > + > + if (priv->model == IP101A) > + return 0; > + > + return __phy_read(phydev, IP101G_PAGE_CONTROL); > +} > + > +static int ip101a_g_write_page(struct phy_device *phydev, int page) > +{ > + struct ip101a_g_phy_priv *priv = phydev->priv; > + > + if (priv->model == IP101A) > + return 0; > + > + return __phy_write(phydev, IP101G_PAGE_CONTROL, page); > +} > + > static struct phy_driver icplus_driver[] = { > { > PHY_ID_MATCH_MODEL(IP175C_PHY_ID), > @@ -373,6 +399,8 @@ static struct phy_driver icplus_driver[] = { > .config_intr = ip101a_g_config_intr, > .handle_interrupt = ip101a_g_handle_interrupt, > .config_init = ip101a_g_config_init, > + .read_page = ip101a_g_read_page, > + .write_page = ip101a_g_write_page, > .soft_reset = genphy_soft_reset, > .suspend = genphy_suspend, > .resume = genphy_resume, >
Hi, Am 2021-02-10 08:03, schrieb Heiner Kallweit: > On 09.02.2021 17:40, Michael Walle wrote: >> Registers >= 16 are paged. Be sure to set the page. It seems this was >> working for now, because the default is correct for the registers used >> in the driver at the moment. But this will also assume, nobody will >> change the page select register before linux is started. The page >> select >> register is _not_ reset with a soft reset of the PHY. >> >> Add read_page()/write_page() support for the IP101G and use it >> accordingly. >> >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- >> drivers/net/phy/icplus.c | 50 >> +++++++++++++++++++++++++++++++--------- >> 1 file changed, 39 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c >> index a6e1c7611f15..858b9326a72d 100644 >> --- a/drivers/net/phy/icplus.c >> +++ b/drivers/net/phy/icplus.c >> @@ -49,6 +49,8 @@ MODULE_LICENSE("GPL"); >> #define IP101G_DIGITAL_IO_SPEC_CTRL 0x1d >> #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32 BIT(2) >> >> +#define IP101G_DEFAULT_PAGE 16 >> + >> #define IP175C_PHY_ID 0x02430d80 >> #define IP1001_PHY_ID 0x02430d90 >> #define IP101A_PHY_ID 0x02430c54 >> @@ -250,23 +252,25 @@ static int ip101a_g_probe(struct phy_device >> *phydev) >> static int ip101a_g_config_init(struct phy_device *phydev) >> { >> struct ip101a_g_phy_priv *priv = phydev->priv; >> - int err; >> + int oldpage, err; >> + >> + oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE); >> >> /* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: >> */ >> switch (priv->sel_intr32) { >> case IP101GR_SEL_INTR32_RXER: >> - err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL, >> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0); >> + err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL, >> + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0); >> if (err < 0) >> - return err; >> + goto out; >> break; >> >> case IP101GR_SEL_INTR32_INTR: >> - err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL, >> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, >> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32); >> + err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL, >> + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, >> + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32); >> if (err < 0) >> - return err; >> + goto out; >> break; >> >> default: >> @@ -284,12 +288,14 @@ static int ip101a_g_config_init(struct >> phy_device *phydev) >> * reserved as 'write-one'. >> */ >> if (priv->model == IP101A) { >> - err = phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS, >> IP101A_G_APS_ON); >> + err = __phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS, >> + IP101A_G_APS_ON); >> if (err) >> - return err; >> + goto out; >> } >> >> - return 0; >> +out: >> + return phy_restore_page(phydev, oldpage, err); > > If a random page was set before entering config_init, do we actually > want > to restore it? Or wouldn't it be better to set the default page as part > of initialization? First, I want to convert this to the match_phy_device() and while at it, I noticed that there is this one "problem". Short summary: the IP101A isn't paged, the IP101G has serveral and if page 16 is selected it is more or less compatible with the IP101A. My problem here is now how to share the functions for both PHYs without duplicating all the code. Eg. the IP101A will phy_read/phy_write/phy_modify(), that is, all the locked versions. For the IP101G I'd either need the _paged() versions or the __phy ones which don't take the mdio_bus lock. Here is what I came up with: (1) provide a common function which uses the __phy ones, then the callback for the A version will take the mdio_bus lock and calls the common one. The G version will use phy_{select,restore}_page(). (2) the phy_driver ops for A will also provde a .read/write_page() callback which is just a no-op. So A can just use the G versions. (3) What Heiner mentioned here, just set the default page in config_init(). (1) will still bloat the code; (3) has the disadvantage, that the userspace might fiddle around with the page register and then the whole PHY driver goes awry. I don't know if we have to respect that use case in general. I know there is an API to read/write the PHY registers and it could happen. That being said, I'm either fine with (2) and (3) but I'm preferring (2). BTW, this patch is still missing read/writes to the interrupt status and control registers which is also paged. -michael
On 10.02.2021 09:25, Michael Walle wrote: > Hi, > > Am 2021-02-10 08:03, schrieb Heiner Kallweit: >> On 09.02.2021 17:40, Michael Walle wrote: >>> Registers >= 16 are paged. Be sure to set the page. It seems this was >>> working for now, because the default is correct for the registers used >>> in the driver at the moment. But this will also assume, nobody will >>> change the page select register before linux is started. The page select >>> register is _not_ reset with a soft reset of the PHY. >>> >>> Add read_page()/write_page() support for the IP101G and use it >>> accordingly. >>> >>> Signed-off-by: Michael Walle <michael@walle.cc> >>> --- >>> drivers/net/phy/icplus.c | 50 +++++++++++++++++++++++++++++++--------- >>> 1 file changed, 39 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c >>> index a6e1c7611f15..858b9326a72d 100644 >>> --- a/drivers/net/phy/icplus.c >>> +++ b/drivers/net/phy/icplus.c >>> @@ -49,6 +49,8 @@ MODULE_LICENSE("GPL"); >>> #define IP101G_DIGITAL_IO_SPEC_CTRL 0x1d >>> #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32 BIT(2) >>> >>> +#define IP101G_DEFAULT_PAGE 16 >>> + >>> #define IP175C_PHY_ID 0x02430d80 >>> #define IP1001_PHY_ID 0x02430d90 >>> #define IP101A_PHY_ID 0x02430c54 >>> @@ -250,23 +252,25 @@ static int ip101a_g_probe(struct phy_device *phydev) >>> static int ip101a_g_config_init(struct phy_device *phydev) >>> { >>> struct ip101a_g_phy_priv *priv = phydev->priv; >>> - int err; >>> + int oldpage, err; >>> + >>> + oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE); >>> >>> /* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: */ >>> switch (priv->sel_intr32) { >>> case IP101GR_SEL_INTR32_RXER: >>> - err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL, >>> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0); >>> + err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL, >>> + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0); >>> if (err < 0) >>> - return err; >>> + goto out; >>> break; >>> >>> case IP101GR_SEL_INTR32_INTR: >>> - err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL, >>> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, >>> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32); >>> + err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL, >>> + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, >>> + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32); >>> if (err < 0) >>> - return err; >>> + goto out; >>> break; >>> >>> default: >>> @@ -284,12 +288,14 @@ static int ip101a_g_config_init(struct phy_device *phydev) >>> * reserved as 'write-one'. >>> */ >>> if (priv->model == IP101A) { >>> - err = phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS, IP101A_G_APS_ON); >>> + err = __phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS, >>> + IP101A_G_APS_ON); >>> if (err) >>> - return err; >>> + goto out; >>> } >>> >>> - return 0; >>> +out: >>> + return phy_restore_page(phydev, oldpage, err); >> >> If a random page was set before entering config_init, do we actually want >> to restore it? Or wouldn't it be better to set the default page as part >> of initialization? > > First, I want to convert this to the match_phy_device() and while at it, > I noticed that there is this one "problem". Short summary: the IP101A isn't > paged, the IP101G has serveral and if page 16 is selected it is more or > less compatible with the IP101A. My problem here is now how to share the > functions for both PHYs without duplicating all the code. Eg. the IP101A > will phy_read/phy_write/phy_modify(), that is, all the locked versions. > For the IP101G I'd either need the _paged() versions or the __phy ones > which don't take the mdio_bus lock. > > Here is what I came up with: > (1) provide a common function which uses the __phy ones, then the > callback for the A version will take the mdio_bus lock and calls > the common one. The G version will use phy_{select,restore}_page(). > (2) the phy_driver ops for A will also provde a .read/write_page() > callback which is just a no-op. So A can just use the G versions. > (3) What Heiner mentioned here, just set the default page in > config_init(). > > (1) will still bloat the code; (3) has the disadvantage, that the > userspace might fiddle around with the page register and then the > whole PHY driver goes awry. I don't know if we have to respect that > use case in general. I know there is an API to read/write the PHY > registers and it could happen. > The potential issue you mention here we have with all PHY's using pages. As one example, the genphy functions rely on the PHY being set to the default page. In general userspace can write PHY register values that break processing, independent of paging. I'm not aware of any complaints regarding this behavior, therefore wouldn't be too concerned here. Regarding (2) I'd like to come back to my proposal from yesterday, implement match_phy_device to completely decouple the A and G versions. Did you consider this option? > That being said, I'm either fine with (2) and (3) but I'm preferring > (2). > > BTW, this patch is still missing read/writes to the interrupt status > and control registers which is also paged. > > -michael Heiner
Hi, Am 2021-02-10 10:03, schrieb Heiner Kallweit: [..] >>>> + return phy_restore_page(phydev, oldpage, err); >>> >>> If a random page was set before entering config_init, do we actually >>> want >>> to restore it? Or wouldn't it be better to set the default page as >>> part >>> of initialization? >> >> First, I want to convert this to the match_phy_device() and while at >> it, >> I noticed that there is this one "problem". Short summary: the IP101A >> isn't >> paged, the IP101G has serveral and if page 16 is selected it is more >> or >> less compatible with the IP101A. My problem here is now how to share >> the >> functions for both PHYs without duplicating all the code. Eg. the >> IP101A >> will phy_read/phy_write/phy_modify(), that is, all the locked >> versions. >> For the IP101G I'd either need the _paged() versions or the __phy ones >> which don't take the mdio_bus lock. >> >> Here is what I came up with: >> (1) provide a common function which uses the __phy ones, then the >> callback for the A version will take the mdio_bus lock and calls >> the common one. The G version will use >> phy_{select,restore}_page(). >> (2) the phy_driver ops for A will also provde a .read/write_page() >> callback which is just a no-op. So A can just use the G versions. >> (3) What Heiner mentioned here, just set the default page in >> config_init(). >> >> (1) will still bloat the code; (3) has the disadvantage, that the >> userspace might fiddle around with the page register and then the >> whole PHY driver goes awry. I don't know if we have to respect that >> use case in general. I know there is an API to read/write the PHY >> registers and it could happen. >> > > The potential issue you mention here we have with all PHY's using > pages. As one example, the genphy functions rely on the PHY being > set to the default page. In general userspace can write PHY register > values that break processing, independent of paging. > I'm not aware of any complaints regarding this behavior, therefore > wouldn't be too concerned here. I'm fine with that, that will make the driver easier. > Regarding (2) I'd like to come back to my proposal from yesterday, > implement match_phy_device to completely decouple the A and G versions. > Did you consider this option? Yes, that is what I was talking about above: "First, I want to convert this to the match_phy_device()" ;) And then I stumbled across that problem I was describing above. It will likely go away if I just set the page to the default page. >> That being said, I'm either fine with (2) and (3) but I'm preferring >> (2). >> >> BTW, this patch is still missing read/writes to the interrupt status >> and control registers which is also paged.
On Wed, Feb 10, 2021 at 08:03:07AM +0100, Heiner Kallweit wrote: > On 09.02.2021 17:40, Michael Walle wrote: > > +out: > > + return phy_restore_page(phydev, oldpage, err); > > If a random page was set before entering config_init, do we actually want > to restore it? Or wouldn't it be better to set the default page as part > of initialization? I think you've missed asking one key question: does the paging on this PHY affect the standardised registers at 0..15 inclusive, or does it only affect registers 16..31? If it doesn't affect the standardised registers, then the genphy_* functions don't care which page is selected.
Am 2021-02-10 11:30, schrieb Russell King - ARM Linux admin: > On Wed, Feb 10, 2021 at 08:03:07AM +0100, Heiner Kallweit wrote: >> On 09.02.2021 17:40, Michael Walle wrote: >> > +out: >> > + return phy_restore_page(phydev, oldpage, err); >> >> If a random page was set before entering config_init, do we actually >> want >> to restore it? Or wouldn't it be better to set the default page as >> part >> of initialization? > > I think you've missed asking one key question: does the paging on this > PHY affect the standardised registers at 0..15 inclusive, or does it > only affect registers 16..31? For this PHY it affects only registers >=16. But that doesn't invaldiate the point that for other PHYs this might affect all regsisters. Eg. ones where you could select between fiber and copper pages, right? > If it doesn't affect the standardised registers, then the genphy_* > functions don't care which page is selected.
On Wed, Feb 10, 2021 at 11:38:18AM +0100, Michael Walle wrote: > Am 2021-02-10 11:30, schrieb Russell King - ARM Linux admin: > > On Wed, Feb 10, 2021 at 08:03:07AM +0100, Heiner Kallweit wrote: > > > On 09.02.2021 17:40, Michael Walle wrote: > > > > +out: > > > > + return phy_restore_page(phydev, oldpage, err); > > > > > > If a random page was set before entering config_init, do we actually > > > want > > > to restore it? Or wouldn't it be better to set the default page as > > > part > > > of initialization? > > > > I think you've missed asking one key question: does the paging on this > > PHY affect the standardised registers at 0..15 inclusive, or does it > > only affect registers 16..31? > > For this PHY it affects only registers >=16. But that doesn't invaldiate > the point that for other PHYs this might affect all regsisters. Eg. ones > where you could select between fiber and copper pages, right? You are modifying the code using ip101a_g_* functions, which is only used for the IP101A and IP101G PHYs. Do these devices support fiber in a way that change the first 16 registers?
Am 2021-02-10 11:49, schrieb Russell King - ARM Linux admin: > On Wed, Feb 10, 2021 at 11:38:18AM +0100, Michael Walle wrote: >> Am 2021-02-10 11:30, schrieb Russell King - ARM Linux admin: >> > On Wed, Feb 10, 2021 at 08:03:07AM +0100, Heiner Kallweit wrote: >> > > On 09.02.2021 17:40, Michael Walle wrote: >> > > > +out: >> > > > + return phy_restore_page(phydev, oldpage, err); >> > > >> > > If a random page was set before entering config_init, do we actually >> > > want >> > > to restore it? Or wouldn't it be better to set the default page as >> > > part >> > > of initialization? >> > >> > I think you've missed asking one key question: does the paging on this >> > PHY affect the standardised registers at 0..15 inclusive, or does it >> > only affect registers 16..31? >> >> For this PHY it affects only registers >=16. But that doesn't >> invaldiate >> the point that for other PHYs this might affect all regsisters. Eg. >> ones >> where you could select between fiber and copper pages, right? > > You are modifying the code using ip101a_g_* functions, which is only > used for the IP101A and IP101G PHYs. Do these devices support fiber > in a way that change the first 16 registers? The PHY doesn't support fiber and register 0..15 are always available regardless of the selected page for the IP101G. genphy_() stuff will work, but the IP101G PHY driver specific functions, like interrupt and mdix will break if someone is messing with the page register from userspace. So Heiner's point was, that there are other PHY drivers which also break when a user changes registers from userspace and no one seemed to cared about that for now. I guess it boils down to: how hard should we try to get the driver behave correctly if the user is changing registers. Or can we just make the assumption that if the PHY driver sets the page selection to its default, all the other callbacks will work on this page. -michael
On Wed, Feb 10, 2021 at 12:14:35PM +0100, Michael Walle wrote: > Am 2021-02-10 11:49, schrieb Russell King - ARM Linux admin: > The PHY doesn't support fiber and register 0..15 are always available > regardless of the selected page for the IP101G. > > genphy_() stuff will work, but the IP101G PHY driver specific functions, > like interrupt and mdix will break if someone is messing with the page > register from userspace. > > So Heiner's point was, that there are other PHY drivers which > also break when a user changes registers from userspace and no one > seemed to cared about that for now. > > I guess it boils down to: how hard should we try to get the driver > behave correctly if the user is changing registers. Or can we > just make the assumption that if the PHY driver sets the page > selection to its default, all the other callbacks will work > on this page. Provided the PHY driver uses the paged accessors for all paged registers, userspace can't break the PHY driver because we have proper locking in the paged accessors (I wrote them.) Userspace can access the paged registers too, but there will be no locking other than on each individual access. This can't be fixed without augmenting the kernel/ user API, and in any case is not a matter for the PHY driver. So, let's stop worrying about the userspace paged access problem for driver reviews; that's a core phylib and userspace API issue. The paged accessor API is designed to allow the driver author to access registers in the most efficient manner. There are two parts to it. 1) the phy_*_paged() accessors switch the page before accessing the register, and restore it afterwards. If you need to access a lot of paged registers, this can be inefficient. 2) phy_save_page()..phy_restore_page() allows wrapping of __phy_* accessors to access paged registers. 3) phy_select_page()..phy_restore_page() also allows wrapping of __phy_* accessors to access paged registers. phy_save_page() and phy_select_page() must /always/ be paired with a call to phy_restore_page(), since the former pair takes the bus lock and the latter releases it. (2) and (3) allow multiple accesses to either a single page without constantly saving and restoring the page, and can also be used to select other pages without the save/restore and locking steps. We could export __phy_read_page() and __phy_write_page() if required. While the bus lock is taken, userspace can't access any PHY on the bus.
Am 2021-02-10 12:48, schrieb Russell King - ARM Linux admin: > On Wed, Feb 10, 2021 at 12:14:35PM +0100, Michael Walle wrote: >> Am 2021-02-10 11:49, schrieb Russell King - ARM Linux admin: >> The PHY doesn't support fiber and register 0..15 are always available >> regardless of the selected page for the IP101G. >> >> genphy_() stuff will work, but the IP101G PHY driver specific >> functions, >> like interrupt and mdix will break if someone is messing with the page >> register from userspace. >> >> So Heiner's point was, that there are other PHY drivers which >> also break when a user changes registers from userspace and no one >> seemed to cared about that for now. >> >> I guess it boils down to: how hard should we try to get the driver >> behave correctly if the user is changing registers. Or can we >> just make the assumption that if the PHY driver sets the page >> selection to its default, all the other callbacks will work >> on this page. > > Provided the PHY driver uses the paged accessors for all paged > registers, userspace can't break the PHY driver because we have proper > locking in the paged accessors (I wrote them.) Userspace can access the > paged registers too, but there will be no locking other than on each > individual access. This can't be fixed without augmenting the kernel/ > user API, and in any case is not a matter for the PHY driver. > > So, let's stop worrying about the userspace paged access problem for > driver reviews; that's a core phylib and userspace API issue. > > The paged accessor API is designed to allow the driver author to access > registers in the most efficient manner. There are two parts to it. > > 1) the phy_*_paged() accessors switch the page before accessing the > register, and restore it afterwards. If you need to access a lot > of paged registers, this can be inefficient. > > 2) phy_save_page()..phy_restore_page() allows wrapping of __phy_* > accessors to access paged registers. > > 3) phy_select_page()..phy_restore_page() also allows wrapping of > __phy_* accessors to access paged registers. > > phy_save_page() and phy_select_page() must /always/ be paired with > a call to phy_restore_page(), since the former pair takes the bus lock > and the latter releases it. > > (2) and (3) allow multiple accesses to either a single page without > constantly saving and restoring the page, and can also be used to > select other pages without the save/restore and locking steps. We > could export __phy_read_page() and __phy_write_page() if required. > > While the bus lock is taken, userspace can't access any PHY on the bus. That was how the v1 of this series was written. But Heiner's review comment was, what if we just set the default page and don't use phy_save_page()..phy_restore_page() for the registers which are behind the default page. And in this case, userspace _can_ break access to the paged registers. -michael
On 10.02.2021 13:17, Michael Walle wrote: > Am 2021-02-10 12:48, schrieb Russell King - ARM Linux admin: >> On Wed, Feb 10, 2021 at 12:14:35PM +0100, Michael Walle wrote: >>> Am 2021-02-10 11:49, schrieb Russell King - ARM Linux admin: >>> The PHY doesn't support fiber and register 0..15 are always available >>> regardless of the selected page for the IP101G. >>> >>> genphy_() stuff will work, but the IP101G PHY driver specific functions, >>> like interrupt and mdix will break if someone is messing with the page >>> register from userspace. >>> >>> So Heiner's point was, that there are other PHY drivers which >>> also break when a user changes registers from userspace and no one >>> seemed to cared about that for now. >>> >>> I guess it boils down to: how hard should we try to get the driver >>> behave correctly if the user is changing registers. Or can we >>> just make the assumption that if the PHY driver sets the page >>> selection to its default, all the other callbacks will work >>> on this page. >> >> Provided the PHY driver uses the paged accessors for all paged >> registers, userspace can't break the PHY driver because we have proper >> locking in the paged accessors (I wrote them.) Userspace can access the >> paged registers too, but there will be no locking other than on each >> individual access. This can't be fixed without augmenting the kernel/ >> user API, and in any case is not a matter for the PHY driver. >> >> So, let's stop worrying about the userspace paged access problem for >> driver reviews; that's a core phylib and userspace API issue. >> >> The paged accessor API is designed to allow the driver author to access >> registers in the most efficient manner. There are two parts to it. >> >> 1) the phy_*_paged() accessors switch the page before accessing the >> register, and restore it afterwards. If you need to access a lot >> of paged registers, this can be inefficient. >> >> 2) phy_save_page()..phy_restore_page() allows wrapping of __phy_* >> accessors to access paged registers. >> >> 3) phy_select_page()..phy_restore_page() also allows wrapping of >> __phy_* accessors to access paged registers. >> >> phy_save_page() and phy_select_page() must /always/ be paired with >> a call to phy_restore_page(), since the former pair takes the bus lock >> and the latter releases it. >> >> (2) and (3) allow multiple accesses to either a single page without >> constantly saving and restoring the page, and can also be used to >> select other pages without the save/restore and locking steps. We >> could export __phy_read_page() and __phy_write_page() if required. >> >> While the bus lock is taken, userspace can't access any PHY on the bus. > > That was how the v1 of this series was written. But Heiner's review > comment was, what if we just set the default page and don't > use phy_save_page()..phy_restore_page() for the registers which are > behind the default page. And in this case, userspace _can_ break > access to the paged registers. > The comment was assuming that paging also applies to register 0..15, like it is the case for Realtek PHY's. That's not the case for your PHY, therefore the situation is slightly different. > -michael
> I guess it boils down to: how hard should we try to get the driver > behave correctly if the user is changing registers. All bets are off if root starts messing with the PHY state from userspace. Its a foot gun, don't be surprised with what happens when you use it. Andrew
diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c index a6e1c7611f15..858b9326a72d 100644 --- a/drivers/net/phy/icplus.c +++ b/drivers/net/phy/icplus.c @@ -49,6 +49,8 @@ MODULE_LICENSE("GPL"); #define IP101G_DIGITAL_IO_SPEC_CTRL 0x1d #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32 BIT(2) +#define IP101G_DEFAULT_PAGE 16 + #define IP175C_PHY_ID 0x02430d80 #define IP1001_PHY_ID 0x02430d90 #define IP101A_PHY_ID 0x02430c54 @@ -250,23 +252,25 @@ static int ip101a_g_probe(struct phy_device *phydev) static int ip101a_g_config_init(struct phy_device *phydev) { struct ip101a_g_phy_priv *priv = phydev->priv; - int err; + int oldpage, err; + + oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE); /* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: */ switch (priv->sel_intr32) { case IP101GR_SEL_INTR32_RXER: - err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL, - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0); + err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL, + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0); if (err < 0) - return err; + goto out; break; case IP101GR_SEL_INTR32_INTR: - err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL, - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32); + err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL, + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32); if (err < 0) - return err; + goto out; break; default: @@ -284,12 +288,14 @@ static int ip101a_g_config_init(struct phy_device *phydev) * reserved as 'write-one'. */ if (priv->model == IP101A) { - err = phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS, IP101A_G_APS_ON); + err = __phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS, + IP101A_G_APS_ON); if (err) - return err; + goto out; } - return 0; +out: + return phy_restore_page(phydev, oldpage, err); } static int ip101a_g_ack_interrupt(struct phy_device *phydev) @@ -347,6 +353,26 @@ static irqreturn_t ip101a_g_handle_interrupt(struct phy_device *phydev) return IRQ_HANDLED; } +static int ip101a_g_read_page(struct phy_device *phydev) +{ + struct ip101a_g_phy_priv *priv = phydev->priv; + + if (priv->model == IP101A) + return 0; + + return __phy_read(phydev, IP101G_PAGE_CONTROL); +} + +static int ip101a_g_write_page(struct phy_device *phydev, int page) +{ + struct ip101a_g_phy_priv *priv = phydev->priv; + + if (priv->model == IP101A) + return 0; + + return __phy_write(phydev, IP101G_PAGE_CONTROL, page); +} + static struct phy_driver icplus_driver[] = { { PHY_ID_MATCH_MODEL(IP175C_PHY_ID), @@ -373,6 +399,8 @@ static struct phy_driver icplus_driver[] = { .config_intr = ip101a_g_config_intr, .handle_interrupt = ip101a_g_handle_interrupt, .config_init = ip101a_g_config_init, + .read_page = ip101a_g_read_page, + .write_page = ip101a_g_write_page, .soft_reset = genphy_soft_reset, .suspend = genphy_suspend, .resume = genphy_resume,
Registers >= 16 are paged. Be sure to set the page. It seems this was working for now, because the default is correct for the registers used in the driver at the moment. But this will also assume, nobody will change the page select register before linux is started. The page select register is _not_ reset with a soft reset of the PHY. Add read_page()/write_page() support for the IP101G and use it accordingly. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/net/phy/icplus.c | 50 +++++++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 11 deletions(-)