| Message ID | YCy1F5xKFJAaLBFw@mwanda (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: icplus: Call phy_restore_page() when phy_select_page() fails | 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 7 of 7 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, 8 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 |
Am 2021-02-17 07:17, schrieb Dan Carpenter: > Smatch warns that there is a locking issue in this function: > > drivers/net/phy/icplus.c:273 ip101a_g_config_intr_pin() > warn: inconsistent returns '&phydev->mdio.bus->mdio_lock'. > Locked on : 242 > Unlocked on: 273 > > It turns out that the comments in phy_select_page() say we have to call > phy_restore_page() even if the call to phy_select_page() fails. > > Fixes: f9bc51e6cce2 ("net: phy: icplus: fix paged register access") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Reviewed-by: Michael Walle <michael@walle.cc> -michael
On Wed, Feb 17, 2021 at 09:17:59AM +0300, Dan Carpenter wrote: > Smatch warns that there is a locking issue in this function: > > drivers/net/phy/icplus.c:273 ip101a_g_config_intr_pin() > warn: inconsistent returns '&phydev->mdio.bus->mdio_lock'. > Locked on : 242 > Unlocked on: 273 > > It turns out that the comments in phy_select_page() say we have to call > phy_restore_page() even if the call to phy_select_page() fails. It seems it's a total waste of time documenting functions...
Am 2021-02-17 11:04, schrieb Russell King - ARM Linux admin: > On Wed, Feb 17, 2021 at 09:17:59AM +0300, Dan Carpenter wrote: >> Smatch warns that there is a locking issue in this function: >> >> drivers/net/phy/icplus.c:273 ip101a_g_config_intr_pin() >> warn: inconsistent returns '&phydev->mdio.bus->mdio_lock'. >> Locked on : 242 >> Unlocked on: 273 >> >> It turns out that the comments in phy_select_page() say we have to >> call >> phy_restore_page() even if the call to phy_select_page() fails. > > It seems it's a total waste of time documenting functions... You once said """ Kernel development is fundamentally a difficult, frustrating and depressing activity. """ But really this comment doesn't make it much better. Yes I've made a mistake although I _read_ the function documentation. So shame on me. -michael
On Wed, Feb 17, 2021 at 11:12:11AM +0100, Michael Walle wrote: > Am 2021-02-17 11:04, schrieb Russell King - ARM Linux admin: > > On Wed, Feb 17, 2021 at 09:17:59AM +0300, Dan Carpenter wrote: > > > Smatch warns that there is a locking issue in this function: > > > > > > drivers/net/phy/icplus.c:273 ip101a_g_config_intr_pin() > > > warn: inconsistent returns '&phydev->mdio.bus->mdio_lock'. > > > Locked on : 242 > > > Unlocked on: 273 > > > > > > It turns out that the comments in phy_select_page() say we have to > > > call > > > phy_restore_page() even if the call to phy_select_page() fails. > > > > It seems it's a total waste of time documenting functions... > > You once said > > """ > Kernel development is fundamentally a difficult, frustrating and > depressing activity. > """ > > But really this comment doesn't make it much better. Yes I've made > a mistake although I _read_ the function documentation. So shame on > me. It wasn't aimed at you - it was more pointing out that in the normal process of kernel development, reading documentation is fairly low, yet we spend time creating it. So, does writing documentation actually help, or does it just slow down the development cycle? Does it have a net positive value? Personally, I don't think it does.
On Wed, Feb 17, 2021 at 09:17:59AM +0300, Dan Carpenter wrote: > Smatch warns that there is a locking issue in this function: > > drivers/net/phy/icplus.c:273 ip101a_g_config_intr_pin() > warn: inconsistent returns '&phydev->mdio.bus->mdio_lock'. > Locked on : 242 > Unlocked on: 273 > > It turns out that the comments in phy_select_page() say we have to call > phy_restore_page() even if the call to phy_select_page() fails. > > Fixes: f9bc51e6cce2 ("net: phy: icplus: fix paged register access") Don't apply this patch. I have created a new Smatch warning for the phy_select_page() behavior and it catches a couple similar bugs in the same file. I will send a v2 that fixes those as well. regards, dan carpenter
On Wed, Feb 17, 2021 at 05:28:38PM +0300, Dan Carpenter wrote: > On Wed, Feb 17, 2021 at 09:17:59AM +0300, Dan Carpenter wrote: > > Smatch warns that there is a locking issue in this function: > > > > drivers/net/phy/icplus.c:273 ip101a_g_config_intr_pin() > > warn: inconsistent returns '&phydev->mdio.bus->mdio_lock'. > > Locked on : 242 > > Unlocked on: 273 > > > > It turns out that the comments in phy_select_page() say we have to call > > phy_restore_page() even if the call to phy_select_page() fails. > > > > Fixes: f9bc51e6cce2 ("net: phy: icplus: fix paged register access") > > Don't apply this patch. I have created a new Smatch warning for the > phy_select_page() behavior and it catches a couple similar bugs in the > same file. I will send a v2 that fixes those as well. Yes, there are three instances of this in the file, all three need fixing. Thanks.
On Wed, Feb 17, 2021 at 03:06:21PM +0000, Russell King - ARM Linux admin wrote: > On Wed, Feb 17, 2021 at 05:28:38PM +0300, Dan Carpenter wrote: > > On Wed, Feb 17, 2021 at 09:17:59AM +0300, Dan Carpenter wrote: > > > Smatch warns that there is a locking issue in this function: > > > > > > drivers/net/phy/icplus.c:273 ip101a_g_config_intr_pin() > > > warn: inconsistent returns '&phydev->mdio.bus->mdio_lock'. > > > Locked on : 242 > > > Unlocked on: 273 > > > > > > It turns out that the comments in phy_select_page() say we have to call > > > phy_restore_page() even if the call to phy_select_page() fails. > > > > > > Fixes: f9bc51e6cce2 ("net: phy: icplus: fix paged register access") > > > > Don't apply this patch. I have created a new Smatch warning for the > > phy_select_page() behavior and it catches a couple similar bugs in the > > same file. I will send a v2 that fixes those as well. > > Yes, there are three instances of this in the file, all three need > fixing. Thanks. I'm wondering whether we need to add __acquires() and __releases() annotations to some of these functions so that sparse can catch these cases. Thoughts?
> I'm wondering whether we need to add __acquires() and __releases() > annotations to some of these functions so that sparse can catch > these cases. Thoughts? Hi Russell The more tools we have for catching locking problems the better. Jakubs patchwork bot should then catch them when a patch is submitted, if the developer did not run sparse themselves. Andrew
On Wed, Feb 17, 2021 at 06:06:48PM +0100, Andrew Lunn wrote: > > I'm wondering whether we need to add __acquires() and __releases() > > annotations to some of these functions so that sparse can catch > > these cases. Thoughts? > > Hi Russell > > The more tools we have for catching locking problems the better. > Jakubs patchwork bot should then catch them when a patch is submitted, > if the developer did not run sparse themselves. Here is how I wrote the check for Smatch. The code in the kernel looks like: oldpage = phy_select_page(phydev, 0x0007); ... phy_restore_page(phydev, oldpage, 0); So what I said is that if phy_select_page() returns an error code then set "phydev" to &selected state. Then if we call phy_restore_page() set it to &undefined. When we hit a return, check if we have any "phydev" variables can possibly be in &selected state and print a warning. The code is below. regards, dan carpenter #include "smatch.h" #include "smatch_slist.h" static int my_id; STATE(selected); static sval_t err_min = { .type = &int_ctype, .value = -4095 }; static sval_t err_max = { .type = &int_ctype, .value = -1 }; static void match_phy_select_page(struct expression *expr, const char *name, struct symbol *sym, void *data) { set_state(my_id, name, sym, &selected); } static void match_phy_restore_page(struct expression *expr, const char *name, struct symbol *sym, void *data) { set_state(my_id, name, sym, &undefined); } static void match_return(struct expression *expr) { struct sm_state *sm; FOR_EACH_MY_SM(my_id, __get_cur_stree(), sm) { if (slist_has_state(sm->possible, &selected)) { sm_warning("phy_select_page() requires restore on error"); return; } } END_FOR_EACH_SM(sm); } void check_phy_select_page_fail(int id) { if (option_project != PROJ_KERNEL) return; my_id = id; return_implies_param_key("phy_select_page", err_min, err_max, &match_phy_select_page, 0, "$", NULL); add_function_param_key_hook("phy_restore_page", &match_phy_restore_page, 0, "$", NULL); add_hook(&match_return, RETURN_HOOK); }
diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c index 4e15d4d02488..015b7b5aa776 100644 --- a/drivers/net/phy/icplus.c +++ b/drivers/net/phy/icplus.c @@ -239,7 +239,7 @@ static int ip101a_g_config_intr_pin(struct phy_device *phydev) oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE); if (oldpage < 0) - return oldpage; + goto out; /* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: */ switch (priv->sel_intr32) {
Smatch warns that there is a locking issue in this function: drivers/net/phy/icplus.c:273 ip101a_g_config_intr_pin() warn: inconsistent returns '&phydev->mdio.bus->mdio_lock'. Locked on : 242 Unlocked on: 273 It turns out that the comments in phy_select_page() say we have to call phy_restore_page() even if the call to phy_select_page() fails. Fixes: f9bc51e6cce2 ("net: phy: icplus: fix paged register access") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/net/phy/icplus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)