Re: [PATCH v4] Add new mac80211 driver mwlwifi.

[Dan Williams <dcbw@redhat.com>]

On Tue, 2015-06-30 at 10:21 +0200, Johannes Berg wrote:
> On Tue, 2015-06-30 at 01:49 +0000, David Lin wrote:
> > +++ b/drivers/net/wireless/mwlwifi/Kconfig
> > @@ -0,0 +1,17 @@
> > +config MWLWIFI
> > +	tristate "Marvell Wireless WiFi driver (mwlwifi)"
> > +	depends on PCI && MAC80211 && MWIFIEX_PCIE=n
> 
> I still think you need to get rid of this so we can build-test this
> driver properly.

The OLPC 8388 is another device that has two drivers, libertas and
libertas_tf.  I don't think there's any protection between then, you get
whatever gets loaded first by the kernel.  In that case, I think the
answer was either (a) only put the driver you want onto the system, or
(b) manually manage from userspace.  Given that this Marvell hardware is
likely intended for more customized use-cases (AP, embedded, etc?)
perhaps this would be an acceptable option for now...

I tend to agree with Johannes here; the builder of the kernel can
certainly adjust CONFIG_MWLWIFI and CONFIG_MWIFIEX to fit their
scenario, including leaving both enabled.

Dan

> > +	select FW_LOADER
> > +	select OF
> 
> This looks OK, though I get a very strange dependency loop warning from
> Kconfig here.
> 
> Looks like the driver now builds almost cleanly with sparse/smatch on
> 64-bit.
> 
> Two warnings remain, both are bugs:
> 
> > writew(0x00, (void __iomem *)&priv->pcmd_buf[1]);
> 
> cannot be right. This memory isn't __iomem, it's dma_alloc_coherent, so
> a simple write should be done.
> 
> in mwl_rx_ring_init:
> 
> >                         rx_hndl->psk_buff =
> >                                 dev_alloc_skb(desc->rx_buf_size);
> > 
> >                         if (skb_linearize(rx_hndl->psk_buff)) {
> 
> *crash*. You also later check rx_hndl->psk_buff, but well after it
> already crashed.
> 
> Also, this code sequence is utterly bogus. Please try to understand why
> and then remove it.
> 
> You should also use paged RX since you're allocating *very large* buffe
> rs. We found that even alloc_pages(1) will fail eventually, you're
> doing an order-2 allocation here for every RX skb. At least used paged
> RX to get it down to order-1.
> 
> johannes
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html