netdev - Re: [RFC PATCH v1 12/19] rtw88: sdio: Add HCI implementation for SDIO based chipsets

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]

Date:   Wed, 28 Dec 2022 12:59:42 +0100
From:   Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To:     Ping-Ke Shih <pkshih@...ltek.com>
Cc:     "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        Yan-Hsuan Chuang <tony0620emma@...il.com>,
        Kalle Valo <kvalo@...nel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        Chris Morgan <macroalpha82@...il.com>,
        Nitin Gupta <nitin.gupta981@...il.com>,
        Neo Jou <neojou@...il.com>,
        Jernej Skrabec <jernej.skrabec@...il.com>
Subject: Re: [RFC PATCH v1 12/19] rtw88: sdio: Add HCI implementation for SDIO
 based chipsets

Hi Ping-Ke,

as always: thank you so much for taking time to go through this!

On Wed, Dec 28, 2022 at 10:39 AM Ping-Ke Shih <pkshih@...ltek.com> wrote:
[...]
>
> > +
> > +static void rtw_sdio_writel(struct rtw_sdio *rtwsdio, u32 val,
> > +                         u32 addr, int *ret)
> > +{
> > +     u8 buf[4];
> > +     int i;
> > +
> > +     if (!(addr & 3) && rtwsdio->is_powered_on) {
> > +             sdio_writel(rtwsdio->sdio_func, val, addr, ret);
> > +             return;
> > +     }
> > +
> > +     *(__le32 *)buf = cpu_to_le32(val);
> > +
> > +     for (i = 0; i < 4; i++) {
> > +             sdio_writeb(rtwsdio->sdio_func, buf[i], addr + i, ret);
> > +             if (*ret)
>
> Do you need some messages to know something wrong?
It's not obvious but we're already logging that something went wrong.
The messages are logged in rtw_sdio_{read,write}{8,16,32}.
We do this because there's multiple ways to access data (direct,
indirect, ...) and some of them require multiple register operations.
So we print one message in the end.

[...]
> > +static u8 rtw_sdio_read_indirect8(struct rtw_dev *rtwdev, u32 addr, int *ret)
> > +{
> > +     struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> > +     u32 reg_cfg, reg_data;
> > +     int retry;
> > +     u8 tmp;
> > +
> > +     reg_cfg = rtw_sdio_to_bus_offset(rtwdev, REG_SDIO_INDIRECT_REG_CFG);
> > +     reg_data = rtw_sdio_to_bus_offset(rtwdev, REG_SDIO_INDIRECT_REG_DATA);
> > +
> > +     rtw_sdio_writel(rtwsdio, BIT(19) | addr, reg_cfg, ret);
> > +     if (*ret)
> > +             return 0;
> > +
> > +     for (retry = 0; retry < RTW_SDIO_INDIRECT_RW_RETRIES; retry++) {
> > +             tmp = sdio_readb(rtwsdio->sdio_func, reg_cfg + 2, ret);
> > +             if (!ret && tmp & BIT(4))
>
> 'ret' is pointer, do you need '*' ?
Well spotted - thank you!

[...]
> As I look into sdio_readb(), it use 'int *err_ret' as arugment.
> Would you like to change ' int *ret' to 'int *err_ret'?
> It could help to misunderstand.
Sure, I'll do that

[...]
> > +             rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, size |
> > +                         (timeout << BIT_SHIFT_DMA_AGG_TO_V1));
>
> BIT_RXDMA_AGG_PG_TH GENMASK(7, 0)       // for size
> BIT_DMA_AGG_TO_V1 GENMASK(15, 8)        // for timeout
Thanks, I'll use these

[...]
> > +static void rtw_sdio_rx_isr(struct rtw_dev *rtwdev)
> > +{
> > +     u32 rx_len;
> > +
> > +     while (true) {
>
> add a limit to prevent infinite loop.
Do you have any recommendations on how many packets to pull in one go?
My thinking is: pulling to little data at once can hurt performance

[...]
>
> > +
> > +static void rtw_sdio_process_tx_queue(struct rtw_dev *rtwdev,
> > +                                   enum rtw_tx_queue_type queue)
> > +{
> > +     struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> > +     struct sk_buff *skb;
> > +     int ret;
> > +
> > +     while (true) {
>
> Can we have a limit?
Similar to the question above: do you have any recommendations on how
many packets (per queue) to send in one go?


Best regards,
Martin