Re: [bug report] drivers/net/phy: add helpers to get/set PLCA configuration

[Dan Carpenter <error27@gmail.com>]

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