* [bug report] drivers/net/phy: add helpers to get/set PLCA configuration
@ 2023-01-16 10:09 Dan Carpenter
2023-01-16 23:11 ` Piergiorgio Beruto
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2023-01-16 10:09 UTC (permalink / raw)
To: piergiorgio.beruto; +Cc: kernel-janitors
Hello Piergiorgio Beruto,
The patch 493323416fed: "drivers/net/phy: add helpers to get/set PLCA
configuration" from Jan 9, 2023, leads to the following Smatch static
checker warning:
drivers/net/phy/phy-c45.c:1036 genphy_c45_plca_set_cfg()
error: uninitialized symbol 'val'.
drivers/net/phy/phy-c45.c
999 int genphy_c45_plca_set_cfg(struct phy_device *phydev,
1000 const struct phy_plca_cfg *plca_cfg)
1001 {
1002 int ret;
1003 u16 val;
1004
1005 // PLCA IDVER is read-only
1006 if (plca_cfg->version >= 0)
1007 return -EINVAL;
1008
1009 // first of all, disable PLCA if required
1010 if (plca_cfg->enabled == 0) {
1011 ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
1012 MDIO_OATC14_PLCA_CTRL0,
1013 MDIO_OATC14_PLCA_EN);
1014
1015 if (ret < 0)
1016 return ret;
1017 }
1018
1019 // check if we need to set the PLCA node count, node ID, or both
1020 if (plca_cfg->node_cnt >= 0 || plca_cfg->node_id >= 0) {
Let's assume both conditions are true
1021 /* if one between node count and node ID is -not- to be
1022 * changed, read the register to later perform merge/purge of
1023 * the configuration as appropriate
1024 */
1025 if (plca_cfg->node_cnt < 0 || plca_cfg->node_id < 0) {
Then neither condition is true here
1026 ret = phy_read_mmd(phydev, MDIO_MMD_VEND2,
1027 MDIO_OATC14_PLCA_CTRL1);
1028
1029 if (ret < 0)
1030 return ret;
1031
1032 val = ret;
1033 }
1034
1035 if (plca_cfg->node_cnt >= 0)
1036 val = (val & ~MDIO_OATC14_PLCA_NCNT) |
^^^
Uninitialized
1037 (plca_cfg->node_cnt << 8);
1038
1039 if (plca_cfg->node_id >= 0)
1040 val = (val & ~MDIO_OATC14_PLCA_ID) |
1041 (plca_cfg->node_id);
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] drivers/net/phy: add helpers to get/set PLCA configuration
2023-01-16 10:09 [bug report] drivers/net/phy: add helpers to get/set PLCA configuration Dan Carpenter
@ 2023-01-16 23:11 ` Piergiorgio Beruto
2023-01-17 3:46 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Piergiorgio Beruto @ 2023-01-16 23:11 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors
On Mon, Jan 16, 2023 at 01:09:19PM +0300, Dan Carpenter wrote:
> Hello Piergiorgio Beruto,
>
> The patch 493323416fed: "drivers/net/phy: add helpers to get/set PLCA
> configuration" from Jan 9, 2023, leads to the following Smatch static
> checker warning:
>
> drivers/net/phy/phy-c45.c:1036 genphy_c45_plca_set_cfg()
> error: uninitialized symbol 'val'.
>
> drivers/net/phy/phy-c45.c
> 999 int genphy_c45_plca_set_cfg(struct phy_device *phydev,
> 1000 const struct phy_plca_cfg *plca_cfg)
> 1001 {
> 1002 int ret;
> 1003 u16 val;
> 1004
> 1005 // PLCA IDVER is read-only
> 1006 if (plca_cfg->version >= 0)
> 1007 return -EINVAL;
> 1008
> 1009 // first of all, disable PLCA if required
> 1010 if (plca_cfg->enabled == 0) {
> 1011 ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
> 1012 MDIO_OATC14_PLCA_CTRL0,
> 1013 MDIO_OATC14_PLCA_EN);
> 1014
> 1015 if (ret < 0)
> 1016 return ret;
> 1017 }
> 1018
> 1019 // check if we need to set the PLCA node count, node ID, or both
> 1020 if (plca_cfg->node_cnt >= 0 || plca_cfg->node_id >= 0) {
>
> Let's assume both conditions are true
>
> 1021 /* if one between node count and node ID is -not- to be
> 1022 * changed, read the register to later perform merge/purge of
> 1023 * the configuration as appropriate
> 1024 */
> 1025 if (plca_cfg->node_cnt < 0 || plca_cfg->node_id < 0) {
>
> Then neither condition is true here
>
> 1026 ret = phy_read_mmd(phydev, MDIO_MMD_VEND2,
> 1027 MDIO_OATC14_PLCA_CTRL1);
> 1028
> 1029 if (ret < 0)
> 1030 return ret;
> 1031
> 1032 val = ret;
> 1033 }
> 1034
> 1035 if (plca_cfg->node_cnt >= 0)
> 1036 val = (val & ~MDIO_OATC14_PLCA_NCNT) |
> ^^^
> Uninitialized
This is correct, however it is not a real problem. Please, allow me to
explain. If both conditions are true (initially) then both conditions at
lines 1035 and 1039 will be true as well. This means that the content of
'val' is completely overwritten with the content of node_cnt and
node_id. This is because the register is 16 bits and node_cnt/id are 8
bits each. Therefore, regardless of the initial status of 'val', its
content is fully determines after line 1039.
With that said, I have no issues in initializing val to 0 if you think
it's still worth it after my explanation.
>
> 1037 (plca_cfg->node_cnt << 8);
> 1038
> 1039 if (plca_cfg->node_id >= 0)
> 1040 val = (val & ~MDIO_OATC14_PLCA_ID) |
> 1041 (plca_cfg->node_id);
>
>
> regards,
> dan carpenter
Thank you very much for reporting this,
Kind Regards,
Piergiorgio
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] drivers/net/phy: add helpers to get/set PLCA configuration
2023-01-16 23:11 ` Piergiorgio Beruto
@ 2023-01-17 3:46 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2023-01-17 3:46 UTC (permalink / raw)
To: Piergiorgio Beruto; +Cc: kernel-janitors
On Tue, Jan 17, 2023 at 12:11:56AM +0100, Piergiorgio Beruto wrote:
> On Mon, Jan 16, 2023 at 01:09:19PM +0300, Dan Carpenter wrote:
> > Hello Piergiorgio Beruto,
> >
> > The patch 493323416fed: "drivers/net/phy: add helpers to get/set PLCA
> > configuration" from Jan 9, 2023, leads to the following Smatch static
> > checker warning:
> >
> > drivers/net/phy/phy-c45.c:1036 genphy_c45_plca_set_cfg()
> > error: uninitialized symbol 'val'.
> >
> > drivers/net/phy/phy-c45.c
> > 999 int genphy_c45_plca_set_cfg(struct phy_device *phydev,
> > 1000 const struct phy_plca_cfg *plca_cfg)
> > 1001 {
> > 1002 int ret;
> > 1003 u16 val;
> > 1004
> > 1005 // PLCA IDVER is read-only
> > 1006 if (plca_cfg->version >= 0)
> > 1007 return -EINVAL;
> > 1008
> > 1009 // first of all, disable PLCA if required
> > 1010 if (plca_cfg->enabled == 0) {
> > 1011 ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
> > 1012 MDIO_OATC14_PLCA_CTRL0,
> > 1013 MDIO_OATC14_PLCA_EN);
> > 1014
> > 1015 if (ret < 0)
> > 1016 return ret;
> > 1017 }
> > 1018
> > 1019 // check if we need to set the PLCA node count, node ID, or both
> > 1020 if (plca_cfg->node_cnt >= 0 || plca_cfg->node_id >= 0) {
> >
> > Let's assume both conditions are true
> >
> > 1021 /* if one between node count and node ID is -not- to be
> > 1022 * changed, read the register to later perform merge/purge of
> > 1023 * the configuration as appropriate
> > 1024 */
> > 1025 if (plca_cfg->node_cnt < 0 || plca_cfg->node_id < 0) {
> >
> > Then neither condition is true here
> >
> > 1026 ret = phy_read_mmd(phydev, MDIO_MMD_VEND2,
> > 1027 MDIO_OATC14_PLCA_CTRL1);
> > 1028
> > 1029 if (ret < 0)
> > 1030 return ret;
> > 1031
> > 1032 val = ret;
> > 1033 }
> > 1034
> > 1035 if (plca_cfg->node_cnt >= 0)
> > 1036 val = (val & ~MDIO_OATC14_PLCA_NCNT) |
> > ^^^
> > Uninitialized
> This is correct, however it is not a real problem. Please, allow me to
> explain. If both conditions are true (initially) then both conditions at
> lines 1035 and 1039 will be true as well. This means that the content of
> 'val' is completely overwritten with the content of node_cnt and
> node_id. This is because the register is 16 bits and node_cnt/id are 8
> bits each. Therefore, regardless of the initial status of 'val', its
> content is fully determines after line 1039.
>
> With that said, I have no issues in initializing val to 0 if you think
> it's still worth it after my explanation.
This issue will be detected at runtime using KMsan. Syzbot will detect
it automatically, complain and involve a lot of developers.
Also there is no downside to initializing to initializing it to zero
because most distros will do this by default now so it likely does not
affect run time.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-01-17 3:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-16 10:09 [bug report] drivers/net/phy: add helpers to get/set PLCA configuration Dan Carpenter
2023-01-16 23:11 ` Piergiorgio Beruto
2023-01-17 3:46 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.