| 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: <CAHp75VeRx8X+5i7SG4KMKADAUj=tkbjfmFDwup5dQ64SLq4yvw@mail.gmail.com>
Date: Thu, 13 Jan 2022 12:16:38 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Joseph CHAMG <josright123@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Rob Herring <robh+dt@...nel.org>, joseph_chang@...icom.com.tw,
netdev <netdev@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Andrew Lunn <andrew@...n.ch>,
Leon Romanovsky <leon@...nel.org>,
kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v11, 2/2] net: Add dm9051 driver
Thanks for update, my comments below.
On Thu, Jan 13, 2022 at 9:46 AM Joseph CHAMG <josright123@...il.com> wrote:
Missed commit message.
...
> v1-v4
> v5
> v6
> v7
> v8
> v9
> v10
Changelog should go after the cutter '--- ' line below, it's strange
that you did it correctly only for v11 changelog and not for the rest.
...
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: Andrew Lunn <andrew@...n.ch>
> Cc: Leon Romanovsky <leon@...nel.org>
> Cc: andy Shevchenko <andy.shevchenko@...il.com>
Instead, utilize --cc parameter to `git send-email ...`
...
> Reported-by: kernel test robot <lkp@...el.com>
New driver can't be reported.
> Signed-off-by: Joseph CHAMG <josright123@...il.com>
> ---
> v11
...
> +config DM9051
> + tristate "DM9051 SPI support"
> + select PHYLIB
> + depends on SPI
> + select CRC32
Please group dependencies first followed by selections.
Also, you missed to select corresponding regmap APIs (SPI and MDIO you
mentioned).
...
> +static int dm9051_map_read(struct board_info *db, u8 reg, unsigned int *prb)
> +{
> + struct net_device *ndev = db->ndev;
> + int ret = regmap_read(db->regmap, reg, prb);
> +
> + if (unlikely(ret))
> + netif_err(db, drv, ndev, "%s: error %d reading reg %02x\n",
> + __func__, ret, reg);
> + return ret;
> +}
> +
> +static int dm9051_map_write(struct board_info *db, u8 reg, u16 val)
> +{
> + struct net_device *ndev = db->ndev;
> + int ret = regmap_write(db->regmap, reg, val);
> +
> + if (unlikely(ret))
> + netif_err(db, drv, ndev, "%s: error %d writing reg %02x=%04x\n",
> + __func__, ret, reg, val);
> + return ret;
> +}
Redefining callbacks for the sake of printing messages? Hmm... dunno
if it's a good idea here.
...
> + ret = dm9051_map_write(db, DM9051_EPDRL, val & 0xff); /* write ctl must once 8-bit */
> + if (ret < 0)
> + return ret;
> + ret = dm9051_map_write(db, DM9051_EPDRH, val >> 8); /* write ctl must once 8-bit */
> + if (ret < 0)
> + return ret;
Wouldn't be better to use bulk write for these?
...
> + ret = dm9051_map_read(db, DM9051_EPDRH, &eph); /* read ctl must once 8-bit */
> + if (ret)
> + return ret;
> + ret = dm9051_map_read(db, DM9051_EPDRL, &epl); /* read ctl must once 8-bit */
> + if (ret)
> + return ret;
> + *val = (eph << 8) | epl;
Wouldn't be better to use bulk read for these?
...
> +static bool dm9051_regmap_volatile(struct device *dev, unsigned int reg)
> +{
> + return true; /* true, register can not be cached */
> +}
Do you need this wrapper?
...
> + .lock = dm9051_reg_lock_mutex,
> + .unlock = dm9051_reg_unlock_mutex,
But this doesn't protect against interleaved accesses. Is it fine?
...
> +static int dm9051_map_updbits(struct board_info *db, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + unsigned int set_mask = val & mask;
> + unsigned int readd = 0; /* clear all insided bits */
> + int ret = 0;
> +
> + ret = regmap_read(db->regmap, reg, &readd);
> + if (ret < 0)
> + return ret;
> +
> + if ((readd & mask) != set_mask) {
> + readd &= ~mask;
> + readd |= set_mask;
> +
> + ret = regmap_write(db->regmap, reg, readd);
> + if (ret < 0)
> + return ret;
> + }
> + return ret;
> +}
NIH regmap_update_bits().
...
> +static bool dm9051_phymap_volatile(struct device *dev, unsigned int reg)
> +{
> + return true; /* true, register can not be cached */
> +}
Do you really need this?
...
> +static int dm9051_map_phy_updbits(struct board_info *db, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + unsigned int set_mask = mask & val;
> + unsigned int readd = 0;
> + int ret;
> +
> + ret = ctrl_dm9051_phyread(db, reg, &readd);
> + if (ret)
> + return ret;
> +
> + if ((readd & mask) != set_mask) {
> + readd &= ~mask;
> + readd |= set_mask;
> + ret = ctrl_dm9051_phywrite(db, reg, readd);
> + if (ret)
> + return ret;
> + }
> + return ret;
> +}
NIH regmap_update_bits().
...
> + ret = dm9051_map_read(db, DM9051_EPDRL, &mval); /* must read once 8-bit */
> + if (ret < 0)
> + return ret;
> + to[0] = mval;
> + ret = dm9051_map_read(db, DM9051_EPDRH, &mval); /* must read once 8-bit */
> + if (ret < 0)
> + return ret;
Why not _bulk operation?
...
> + dm9051_map_write(db, DM9051_EPDRH, data[1]); /* must write once 8-bit */
> + dm9051_map_write(db, DM9051_EPDRL, data[0]); /* must write once 8-bit */
Ditto.
...
> + ret = dm9051_map_read(db, DM9051_PIDH, &wpidh);
> + if (ret < 0)
> + return ret;
> + ret = dm9051_map_read(db, DM9051_PIDL, &wpidl);
> + if (ret < 0)
> + return ret;
> + id = (wpidh << 8) | wpidl;
Ditto.
...
> + if (id == DM9051_ID) {
> + dev_info(dev, "chip %04x found\n", id);
> + return 0;
> + }
> +
> + dev_info(dev, "chipid error as %04x !\n", id);
Why not dev_err()?
> + return -ENODEV;
Please, use standard pattern, i.e. check for errors first:
if (error condition) {
...
return err;
}
...
> + for (i = 0; i < ETH_ALEN; i++) {
> + ret = dm9051_map_read(db, DM9051_PAR + i, &mval);
> + if (unlikely(ret))
> + return ret;
> + addr[i] = mval;
> + }
_bulk?
...
> + if (is_valid_ether_addr(addr)) {
> + eth_hw_addr_set(ndev, addr);
> + return 0;
> + }
> +
> + eth_hw_addr_random(ndev);
> + dev_dbg(&db->spidev->dev, "Use random MAC address\n");
> + return 0;
Check for (kinda) error first.
...
> + snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT,
> + db->mdiobus->id, DM9051_PHY_ID);
(1)
> + db->phydev = phy_connect(db->ndev, phy_id, dm9051_handle_link_change,
> + PHY_INTERFACE_MODE_MII);
> +
This blank line is misplaced. Should be in (1).
> + if (IS_ERR(db->phydev))
> + return PTR_ERR(db->phydev);
> + return 0;
return PTR_ERR_OR_ZERO(...);
> +}
...
> + rdptr = (u8 *)skb_put(skb, rxlen - 4);
Do you need this casting?
...
> + ret = dm9051_map_write(db, DM9051_TXPLL, len);
> + if (ret < 0)
> + return ret;
> + ret = dm9051_map_write(db, DM9051_TXPLH, len >> 8);
> + if (ret < 0)
> + return ret;
_bulk?
...
> + /* these registers can't write by inblk, must write one by one
> + */
Why? regmap bulk does exactly this (if properly configured).
> + for (i = 0; i < ETH_ALEN; i++) {
> + result = dm9051_map_write(db, DM9051_PAR + i, ndev->dev_addr[i]);
> + if (result < 0)
> + goto spi_err;
> + }
...
> + /* these registers can't write by inblk, must write one by one
> + */
Ditto.
> + for (oft = DM9051_MAR, i = 0; i < 4; i++) {
> + result = dm9051_map_write(db, oft++, db->hash_table[i]);
> + if (result < 0)
> + goto spi_err;
> + result = dm9051_map_write(db, oft++, db->hash_table[i] >> 8);
> + if (result < 0)
> + goto spi_err;
> + }
...
> + db->hash_table[hash_val / 16] |= (u16)1 << (hash_val % 16);
Do you need casting? Can you use 1U or BIT()?
...
> +static int devm_regmap_init_dm9051(struct spi_device *spi, struct board_info *db)
> +{
> + regconfig.lock_arg = db; /* regmap lock/unlock essential */
> +
> + db->regmap = devm_regmap_init_spi(db->spidev, ®config);
> + if (IS_ERR(db->regmap))
> + return PTR_ERR(db->regmap);
> + return 0;
return PTR_ERR_OR_ZERO(...);
> +}
...
> + db->mdiobus->phy_mask = (u32)~BIT(1);
GENMASK()
...
> + ret = devm_mdiobus_register(&spi->dev, db->mdiobus);
> + if (ret < 0) {
What does > 0 mean?
> + dev_err(&spi->dev, "Could not register MDIO bus\n");
> + return ret;
> + }
> + return 0;
Can it be
return ret;
?
...
> + if (IS_ERR(db->phymap))
> + return PTR_ERR(db->phymap);
> + return 0;
As above.
...
> + db->kwr_task_kw = kthread_run(kthread_worker_fn, &db->kw, "dm9051");
> + if (IS_ERR(db->kwr_task_kw))
> + return PTR_ERR(db->kwr_task_kw);
> +
> + mutex_init(&db->spi_lock);
> + mutex_init(&db->reg_mutex);
Are you sure it's good to have thread running without initializations
of locks, etc?
...
> +static int dm9051_drv_remove(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct board_info *db = to_dm9051_board(ndev);
> +
> + phy_disconnect(db->phydev);
> + kthread_stop(db->kwr_task_kw);
> + return 0;
> +}
Seems like a wrong order of the resource freeing.
...
> +++ b/drivers/net/ethernet/davicom/dm9051.h
Do oyu need it to be a separate header?
...
> +#include <linux/bitfield.h>
Not sure I saw the user of this header below.
> +#include <linux/mutex.h>
> +#include <linux/netdevice.h>
...
> +struct rx_ctl_mach {
> + u16 status_err_counter; /* 'Status Err' counter */
> + u16 large_err_counter; /* 'Large Err' counter */
> + u16 DO_FIFO_RST_counter; /* 'fifo_reset' counter */
Can you rather have these comments in kernel doc?
> +};
...
> +struct board_info
Why do you need this definition in the header?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists