netdev - Re: [PATCH v4 3/5] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201029003131.GF933237@lunn.ch>
Date:   Thu, 29 Oct 2020 01:31:31 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Łukasz Stelmach <l.stelmach@...sung.com>
Cc:     jim.cromie@...il.com, Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Kukjin Kim <kgene@...nel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org,
        Bartłomiej Żolnierkiewicz 
        <b.zolnierkie@...sung.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>
Subject: Re: [PATCH v4 3/5] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter
 Driver

> +static void
> +ax88796c_get_regs(struct net_device *ndev, struct ethtool_regs *regs, void *_p)
> +{
> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
> +	u16 *p = _p;
> +	int offset, i;

You missed a reverse christmass tree fix here.

> +static int comp;
> +static int msg_enable = NETIF_MSG_PROBE |
> +			NETIF_MSG_LINK |
> +			/* NETIF_MSG_TIMER | */
> +			/* NETIF_MSG_IFDOWN | */
> +			/* NETIF_MSG_IFUP | */
> +			NETIF_MSG_RX_ERR |
> +			NETIF_MSG_TX_ERR |
> +			/* NETIF_MSG_TX_QUEUED | */
> +			/* NETIF_MSG_INTR | */
> +			/* NETIF_MSG_TX_DONE | */
> +			/* NETIF_MSG_RX_STATUS | */
> +			/* NETIF_MSG_PKTDATA | */
> +			/* NETIF_MSG_HW | */
> +			/* NETIF_MSG_WOL | */
> +			0;

You should probably delete anything which is commented out.

> +
> +static char *no_regs_list = "80018001,e1918001,8001a001,fc0d0000";
> +unsigned long ax88796c_no_regs_mask[AX88796C_REGDUMP_LEN / (sizeof(unsigned long) * 8)];
> +
> +module_param(comp, int, 0444);
> +MODULE_PARM_DESC(comp, "0=Non-Compression Mode, 1=Compression Mode");

I think you need to find a different way to configure this. How much
does compression bring you anyway?

> +module_param(msg_enable, int, 0444);
> +MODULE_PARM_DESC(msg_enable, "Message mask (see linux/netdevice.h for bitmap)");

I know a lot of drivers have msg_enable, but DaveM is generally
against module parameters. So i would remove this.


> +static void ax88796c_set_hw_multicast(struct net_device *ndev)
> +{
> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
> +	u16 rx_ctl = RXCR_AB;
> +	int mc_count = netdev_mc_count(ndev);

reverse christmass tree.

> +static struct sk_buff *
> +ax88796c_tx_fixup(struct net_device *ndev, struct sk_buff_head *q)
> +{
> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
> +	struct sk_buff *skb, *tx_skb;
> +	struct tx_pkt_info *info;
> +	struct skb_data *entry;
> +	int headroom;
> +	int tailroom;
> +	u8 need_pages;
> +	u16 tol_len, pkt_len;
> +	u8 padlen, seq_num;
> +	u8 spi_len = ax_local->ax_spi.comp ? 1 : 4;

reverse christmass tree.

> +static int ax88796c_receive(struct net_device *ndev)
> +{
> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
> +	struct sk_buff *skb;
> +	struct skb_data *entry;
> +	u16 w_count, pkt_len;
> +	u8 pkt_cnt;

Reverse christmass tree

> +
> +static int ax88796c_process_isr(struct ax88796c_device *ax_local)
> +{
> +	u16 isr;
> +	u8 done = 0;
> +	struct net_device *ndev = ax_local->ndev;

...

> +static irqreturn_t ax88796c_interrupt(int irq, void *dev_instance)
> +{
> +	struct net_device *ndev = dev_instance;
> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);

...

> +static int
> +ax88796c_open(struct net_device *ndev)
> +{
> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
> +	int ret;
> +	unsigned long irq_flag = IRQF_SHARED;
> +	int fc = AX_FC_NONE;

...


> +static int ax88796c_probe(struct spi_device *spi)
> +{
> +	struct net_device *ndev;
> +	struct ax88796c_device *ax_local;
> +	char phy_id[MII_BUS_ID_SIZE + 3];
> +	int ret;
> +	u16 temp;

...

The mdio/phy/ethtool code looks O.K. now. I've not really looked at
any of the frame transfer code, so i cannot comment on that.

    Andrew