RE: [PATCH 1/7] ath10k: enable RX bundle receive for sdio

From: Wen Gong <wgong@qti.qualcomm.com>
To: Nicolas Boichat <drinkcat@chromium.org>, Wen Gong <wgong@codeaurora.org>
Cc: "open list:NETWORKING DRIVERS (WIRELESS)" 
	<linux-wireless@vger.kernel.org>,
	"ath10k@lists.infradead.org" <ath10k@lists.infradead.org>
Subject: RE: [PATCH 1/7] ath10k: enable RX bundle receive for sdio
Date: Tue, 27 Aug 2019 08:02:52 +0000	[thread overview]
Message-ID: <2ec213d88ebb4b89812826866b39886d@aptaiexm02f.ap.qualcomm.com> (raw)
In-Reply-To: <CANMq1KBR0nd+XOJWpgWyoJdsFgZByQyjD-0dpttygiN0OfiD5Q@mail.gmail.com>

> -----Original Message-----
> From: ath10k <ath10k-bounces@lists.infradead.org> On Behalf Of Nicolas
> Boichat
> Sent: Tuesday, August 27, 2019 3:41 PM
> To: Wen Gong <wgong@codeaurora.org>
> Cc: open list:NETWORKING DRIVERS (WIRELESS) <linux-
> wireless@vger.kernel.org>; ath10k@lists.infradead.org
> Subject: [EXT] Re: [PATCH 1/7] ath10k: enable RX bundle receive for sdio
> 
> > -static int ath10k_sdio_mbox_rx_fetch(struct ath10k *ar)
> > +static int ath10k_sdio_mbox_rx_fetch_bundle(struct ath10k *ar)
> >  {
> >         struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
> > +       struct ath10k_sdio_rx_data *pkt;
> >         int ret, i;
> > +       u32 pkt_offset, virt_pkt_len;
> >
> > +       virt_pkt_len = 0;
> >         for (i = 0; i < ar_sdio->n_rx_pkts; i++) {
> > -               ret = ath10k_sdio_mbox_rx_packet(ar,
> > -                                                &ar_sdio->rx_pkts[i]);
> > -               if (ret)
> > +               virt_pkt_len += ar_sdio->rx_pkts[i].alloc_len;
> > +       }
> > +
> > +       if (virt_pkt_len < ATH10K_SDIO_DMA_BUF_SIZE) {
> > +               ret = ath10k_sdio_readsb(ar, ar_sdio->mbox_info.htc_addr,
> > +                                        ar_sdio->vsg_buffer, virt_pkt_len);
> > +               if (ret) {
> > +                       i = 0;
> >                         goto err;
> > +               }
> > +       } else {
> > +               ath10k_err(ar, "size exceeding limit %d\n", virt_pkt_len);
> > +       }
> > +
> > +       pkt_offset = 0;
> > +       for (i = 0; i < ar_sdio->n_rx_pkts; i++) {
> > +               struct sk_buff *skb = ar_sdio->rx_pkts[i].skb;
> > +
> > +               pkt = &ar_sdio->rx_pkts[i];
> > +               memcpy(skb->data, ar_sdio->vsg_buffer + pkt_offset,
> > +                      pkt->alloc_len);
> 
> Why do you copy alloc_len if you only plan to use act_len?
alloc_len is aligned to block size(256),  and act_len maybe not same for each packet in the bundle.
Eg a bundle:
Packet 1 len 240,   alloc_len: 256, 1st time: act_len 240, left padding size is 16,
Packet 1 len 250, alloc_len: 256, 1st time: act_len 240, left padding size is 6,
Packet 1 len 230,   alloc_len: 256, 1st time: act_len 240, left padding size is 26,

The bundled buffer len is 256 * 3 = 768, it has 256 bytes for each packet, the left size is padding which 
Is not needed, but the left padding is not same for each packet, before read all the buffer from sdio bus,
It does not know each packet's act len, it only know the 1st packet's act len.
So it need to copy all the alloc_len's buffer to ensure it will not lose data.

> Actually, just use skb_put_data.
> 
> Also, do you have the same issue as
> https://patchwork.kernel.org/patch/11116215/ w.r.t. act_len being
> incorrect?
> 
> > +               pkt->status = 0;
> > +               skb_put(skb, pkt->act_len);
> > +               pkt_offset += pkt->alloc_len;
> >         }
> >
> >         return 0;
> >
> 
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k