Re: [PATCH 0/2] enable 802.11w in mt7601u driver

[Jakub Kicinski]

On Mon,  9 Jul 2018 12:20:25 +0200, Lorenzo Bianconi wrote:
> Lorenzo Bianconi (1):
>   mt7601u: use sw encryption for hw unsupported ciphers
> 
> Davide Caratti (1):
>   mt7601u: expose 802.11w support
> 
>  drivers/net/wireless/mediatek/mt7601u/init.c |  1 +
>  drivers/net/wireless/mediatek/mt7601u/main.c | 11 +++
>  2 files changed, 12 insertions(+)

Acked-by: Jakub Kicinski 

Re: [PATCH] mt7601u: remove warning when avg_rssi is zero

[Jakub Kicinski]

On Sat, 9 Jun 2018 12:10:44 +0200, Stanislaw Gruszka wrote:
> It turned out that we can run calibration without already received
> frames with RSSI data. In such case just don't update AGC register
> and don't print the warning.
> 
> Fixes: b305a6ab0247 ("mt7601u: use EWMA to calculate avg_rssi")
> Reported-by: Adam Borowski 
> Signed-off-by: Stanislaw Gruszka 

Acked-by: Jakub Kicinski 

Re: [PATCH 2/2 v2] mt7601u: run calibration works after finishing scanning

[Jakub Kicinski]

On Mon, 16 Apr 2018 13:56:18 +0200, Stanislaw Gruszka wrote:
> When finishing scanning we switch to operational channel sill with
> SCANNING flag. This mean that we never perform calibration works after
> scanning. To fix the problem queue calibration works on
> .sw_scan_complete() routine.
> 
> Signed-off-by: Stanislaw Gruszka <sgrus...@redhat.com>

Acked-by: Jakub Kicinski <kubak...@wp.pl>

Thank you!!

Re: [PATCH 1/2] mt7601u: use EWMA to calculate avg_rssi

[Jakub Kicinski]

On Fri, 13 Apr 2018 16:44:37 +0200, Stanislaw Gruszka wrote:
> avr_rssi is not calculated correctly as we do not divide result
> by 256 (mt76 sum avg_rssi1 and avg_rssi2 and divide by 512).
> However dividing by 256 will make avg_rssi almost the same as
> last rssi value - not really an average. So use EWMA to calculate
> avg_rssi. I've chosen weight_rcp=4 to convergence quicker on signal
> strength changes.
> 
> Signed-off-by: Stanislaw Gruszka <sgrus...@redhat.com>

Acked-by: Jakub Kicinski <kubak...@wp.pl>

Re: [PATCH 2/2] mt7601u: run calibration works after finishing scanning

[Jakub Kicinski]

On Fri, 13 Apr 2018 16:44:38 +0200, Stanislaw Gruszka wrote:
> When finishing scanning we switch to operational channel sill with
> SCANNING flag. This mean that we never perform calibration works after
> scanning. To fix the problem cancel and queue calibration works on
> .sw_scan_start() and .sw_scan_complete() routines.
> 
> Signed-off-by: Stanislaw Gruszka 

IOW the stack will potentially ask us to return to the original channel
before calling .sw_scan_complete()?  Hm.  That's unpleasant.

I'm not entirely on board with the patch.  Now normal channel setting
will happen while calibration is running.  Is that necessary?  Would it
not be sufficient to make sure work is scheduled
from .sw_scan_complete()?

Re: [PATCH] mt7601u: phy: mark expected switch fall-through

[Jakub Kicinski]

On Fri, 30 Mar 2018 16:12:23 -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
> 
> Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>

Acked-by: Jakub Kicinski <kubak...@wp.pl>

Re: [PATCH v3 20/20] mt7601u: use request_firmware_cache() to address cache on reboot

[Jakub Kicinski]

On Sat, 10 Mar 2018 06:15:01 -0800, Luis R. Rodriguez wrote:
> request_firmware_cache() will ensure the firmware is available on resume
> from suspend if on reboot the device retains the firmware.
> 
> This optimization is in place given otherwise on reboot we have to
> reload the firmware, the opmization saves us about max 1s, minimum 10ms.
> 
> Cantabile has reported back this fixes his woes with both suspend and
> hibernation.
> 
> Reported-by: Cantabile <cantabile.d...@gmail.com>
> Tested-by: Cantabile <cantabile.d...@gmail.com>
> Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>

FWIW:

Acked-by: Jakub Kicinski <kubak...@wp.pl>

Thanks Luis!!

Re: [PATCH] mt7601u: simplify mt7601u_mcu_msg_alloc signature

[Jakub Kicinski]

On Wed,  7 Mar 2018 10:25:50 +0100, Lorenzo Bianconi wrote:
> Remove mt7601u_dev parameter from mt7601u_mcu_msg_alloc signature since
> dev pointer is never used in routine body
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>

Acked-by: Jakub Kicinski <kubak...@wp.pl>

Re: [PATCH] mt7601u: remove a warning in mt7601u_efuse_physical_size_check()

[Jakub Kicinski]

On Wed, 28 Feb 2018 15:26:57 +0100, Lorenzo Bianconi wrote:
> Fix the following sparse warning in mt7601u_efuse_physical_size_check:
> - drivers/net/wireless/mediatek/mt7601u/eeprom.c:77:27: warning:
>   Variable length array is used
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>

Acked-by: Jakub Kicinski <kubak...@wp.pl>

Thanks Lorenzo!

Re: [PATCH] mt7601u: Fix system freeze after resuming from hibernation

[Jakub Kicinski]

On Tue, 27 Feb 2018 16:54:51 +, Luis R. Rodriguez wrote:
> On Tue, Feb 27, 2018 at 02:25:55PM +0200, cantabile wrote:
> > On 27/02/18 04:28, Jakub Kicinski wrote:  
> > > On Sun, 25 Feb 2018 17:54:25 +, Luis R. Rodriguez wrote:  
> >   
> > > > I want to understand the case where the firmware is *not* available on 
> > > > resume?
> > > > Why did that happen? I seem to have read that on a fresh reboot the 
> > > > firmware
> > > > was not needed, and so on probe request_firmware() was not called? Why 
> > > > would
> > > > firmware not be required on a reboot?  
> > > 
> > > Yes, that is a good question..  John, do you have a theory?  My initial
> > > thought was that the UEFI/BIOS loads it during pre-boot, but this is a
> > > USB card, so it's a bit unlikely that UEFI will have a driver for it...
> > > Does this happen when rebooting maybe?
> > 
> > Yes, it happens when rebooting:
> > 1) Plug in the dongle. Message about firmware appears in dmesg:
> > 
> > mt7601u 2-3:1.0: ASIC revision: 76010001 MAC revision: 76010500
> > mt7601u 2-3:1.0: Firmware Version: 0.1.00 Build: 7640 Build time:
> > 201302052146
> > mt7601u 2-3:1.0: Warning: unsupported EEPROM version 0d
> > 
> > 2) `systemctl reboot`. Message about firmware does not appear in dmesg:
> > 
> > mt7601u 2-3:1.0: ASIC revision: 76010001 MAC revision: 76010500
> > mt7601u 2-3:1.0: Warning: unsupported EEPROM version 0d
> > 
> > The dongle is nevertheless perfectly functional after rebooting.
> > 
> > I have no idea why it works like this.
> > 
> > This is an older laptop, no UEFI.  
>
> OK, this just confirms that firmware is not needed on reboot sometimes,
> but it does not explain *why*. What driver and code lines are involved
> so I can go read? 

mt7601u_load_firmware() is probably the place to look at.

> Is the vendor involved or not really?

Not really, we got the vendor to submit the FW to linux-firmware,
that's about it.

> I could imagine a situation where on reboot we just reset a device but
> since poweroff is never fully issued the firmware is not lost. So let me
> ask, why not do a full reset / shutdown of the device when the interface
> goes down?
> 
> If there is no gain from the behaviour observed, might as well make
> the device work as much others rather than adding an API for a one-off
> situation where there is no gain from it behaving in a unique way.

IDK what the reset procedure is :S

Re: [PATCH] mt7601u: Fix system freeze after resuming from hibernation

[Jakub Kicinski]

On Sun, 25 Feb 2018 17:54:25 +, Luis R. Rodriguez wrote:
> On Mon, Feb 19, 2018 at 05:01:27PM +0200, cantabile wrote:
> > On 19/02/18 07:55, Jakub Kicinski wrote:  
> > > On Sat, 17 Feb 2018 13:23:29 +0200, cantabile wrote:  
> > > > > Thanks for the info.  Would it be cleaner to EXPORT fw_add_devm_name()
> > > > > and just call that in case driver sees FW is already loaded?  That
> > > > > should inform the fw subsystem that we want the image around in case 
> > > > > of
> > > > > hibernation, but there is no need to load it immediately?  
> > > > 
> > > > No, I don't believe it's cleaner to expose a private function that you
> > > > don't even really need. Remember that calling request_firmware every
> > > > time your driver's probe and resume functions are called is normal. It's
> > > > the expected behaviour.  
> > > 
> > > I'm asking you the extend functionality of a subsystem to be able to
> > > cleanly communicate the intent.  Not export internal functions.
> > > 
> > > Requesting firmware you don't need and risking failing probe even if FW
> > > is already pre-loaded is not correct.  Reordering you suggest is
> > > brittle and makes little logical sense unless someone guesses your use
> > > case.
> > > 
> > > Please at least try to do as advised.  Otherwise:
> > > 
> > > Nacked-by: Jakub Kicinski <kubak...@wp.pl>
> > >   
> > 
> > You're right about the reordering not making sense to someone unfamiliar
> > with the problem. I can fix that with a comment.
> > 
> > I can change the patch so that request_firmware will only make the probe
> > function fail if the firmware is not already running.  
> 
> Note that using request_firmware() on probe typically is also not an
> outstanding idea given it delays boot. Not because looking for the firmware
> takes time, but instead because processing firmware typically does on
> the device. For instance cxgb4 is an example device where processing
> firmware takes a long time.
> 
> Delays on probe may mean the "feel good" immediate desktop coming up is 
> delyed.
> 
> Specially if its networking... I see no reason why to process firmware on 
> probe.
> 
> If one can use a workqueue to process verifying if it needs firmware and 
> loading
> later, that's more advisable.

Quite true, more advanced the FW the longer FW load takes :(  Although
I would be cautious not to cause issues for network/NFS boot...  Perhaps
it can wait for such workqueue to finish?

> Now, that's all a side topic.
> 
> I will for now agree that it seems pointless to request for firmware always
> even if you don't need to, and all you want is to just cache the firmware
> on suspend. So I welcome a patch but the justification for it really needs to
> be documented very well, and the documentation extended as such. In fact
> maybe rename the function to something more sensible.
> 
> Another use case for the firmware cache (which we need to add the 
> documentation)
> is that for hibernation we suspend all devices first, get a snapshot, and then
> resume devices so we can then write the snapshot to disk. On that resume step
> I don't think devices have access to the hard drive for firmware, so cache is
> all we have. This may need some confirmation but I suspect this is the case.
> Drivers needing firmware on resume for hibernation may need to cache their
> firmware.
> 
> I want to understand the case where the firmware is *not* available on resume?
> Why did that happen? I seem to have read that on a fresh reboot the firmware
> was not needed, and so on probe request_firmware() was not called? Why would
> firmware not be required on a reboot?

Yes, that is a good question..  John, do you have a theory?  My initial
thought was that the UEFI/BIOS loads it during pre-boot, but this is a
USB card, so it's a bit unlikely that UEFI will have a driver for it...
Does this happen when rebooting maybe?

Re: [PATCH] mt7601u: Fix system freeze after resuming from hibernation

[Jakub Kicinski]

On Sat, 17 Feb 2018 13:23:29 +0200, cantabile wrote:
> > Thanks for the info.  Would it be cleaner to EXPORT fw_add_devm_name()
> > and just call that in case driver sees FW is already loaded?  That
> > should inform the fw subsystem that we want the image around in case of
> > hibernation, but there is no need to load it immediately?
> 
> No, I don't believe it's cleaner to expose a private function that you 
> don't even really need. Remember that calling request_firmware every 
> time your driver's probe and resume functions are called is normal. It's 
> the expected behaviour.

I'm asking you the extend functionality of a subsystem to be able to
cleanly communicate the intent.  Not export internal functions.

Requesting firmware you don't need and risking failing probe even if FW
is already pre-loaded is not correct.  Reordering you suggest is
brittle and makes little logical sense unless someone guesses your use
case.

Please at least try to do as advised.  Otherwise:

Nacked-by: Jakub Kicinski <kubak...@wp.pl>

Re: [PATCH v2] mt7601u: make write with mask access atomic

[Jakub Kicinski]

On Fri, 16 Feb 2018 23:30:01 +0100, Lorenzo Bianconi wrote:
> Introduce __mt7601u_rr and __mt7601u_vendor_single_wr routines in order
> to make mt7601u_rmw and mt7601u_rmc atomic. This patch does not fix a
> reported issue but makes the usb access more robust to concurrent
> operations on the same register since it is theoretically possible that
> read and write accesses of mt7601u_rmw/mt7601u_rmc can be interleaved with
> a different write operation on the same register.
> Moreover using __mt7601u_rr and __mt7601u_vendor_single_wr in
> mt7601u_rmw/mt7601u_rmc allows to grab vendor_req_mutex mutex once
> instead of twice
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>

Acked-by: Jakub Kicinski <kubak...@wp.pl>

Thanks!

Re: [PATCH] mt7601u: make write with mask access atomic

[Jakub Kicinski]

On Fri, 16 Feb 2018 10:06:49 +0100, Lorenzo Bianconi wrote:
> > Hm.. There should be no path in the driver where that could cause
> > problems AFAIR.  We have reg_atomic_mutex and hw_atomic_mutex to
> > protect from concurrent access to the HW where they are possible.
> > RMW operations are non-atomic by definition, it's supposed to work 
> > like PCIe register accesses would - 32bit reads/writes are atomic, 
> > but RMW is not.  
> 
> Yes, RMW accesses are non-atomic by default but since vendor_req_mutex mutex
> is already there (and grabbed for RMW operations), why not use it to make 
> write
> with mask access atomic without adding complexity? Moreover it would be a
> micro-optimisation since vendor_req_mutex would be grabbed just once instead 
> of
> twice
> 
> > 
> > So I'm not sure what to do with this patch.  Doesn't seem necessary...  
> 
> It is just a trivial rework of locking in usb read/write accesses, not
> mandatory, so if you prefer we can just drop it

You make good points :)  Could you please respin stating clearly in the
commit message that this is not a bug fix, but a micro-optimization and
may be useful in the future?

Re: [PATCH] mt7601u: make write with mask access atomic

[Jakub Kicinski]

On Fri, 16 Feb 2018 01:02:31 +0100, Lorenzo Bianconi wrote:
> > On Thu, 15 Feb 2018 23:59:24 +0100, Lorenzo Bianconi wrote:  
> >> Introduce __mt7601u_rr and __mt7601u_vendor_single_wr routines in order
> >> to make mt7601u_rmw and mt7601u_rmc atomic since it is possible that
> >> read and write accesses of mt7601u_rmw/mt7601u_rmc can be interleaved
> >> with a different write operation on the same register.
> >> Moreover move write trace point in __mt7601u_vendor_single_wr
> >>
> >> Signed-off-by: Lorenzo Bianconi   
> >
> > Could you provide an example of which accesses make it problematic?
> > Is this fixing an actual bug?  
> 
> Hi Jakub,
> 
> it is not an issue I had experimented, I noticed a theoretical race
> reviewing the code.
> AFAIU, based on the current implementation it is possible that mt7601u_rmw
> (with mt7601u_rr) loads data from given register but its store access
> (mt7601u_wr) is
> preceded by another mt7601u_wr on the same register. In this case the
> value configured by
> the first mt7601u_wr executed is overwritten by the second one (the
> store from mt7601u_rmw)
> even if the first write is setting a different subfield respect to
> mt7601u_rmw.

Hm.. There should be no path in the driver where that could cause
problems AFAIR.  We have reg_atomic_mutex and hw_atomic_mutex to
protect from concurrent access to the HW where they are possible.
RMW operations are non-atomic by definition, it's supposed to work 
like PCIe register accesses would - 32bit reads/writes are atomic, 
but RMW is not.

So I'm not sure what to do with this patch.  Doesn't seem necessary...

Re: [PATCH] mt7601u: make write with mask access atomic

[Jakub Kicinski]

On Thu, 15 Feb 2018 23:59:24 +0100, Lorenzo Bianconi wrote:
> Introduce __mt7601u_rr and __mt7601u_vendor_single_wr routines in order
> to make mt7601u_rmw and mt7601u_rmc atomic since it is possible that
> read and write accesses of mt7601u_rmw/mt7601u_rmc can be interleaved
> with a different write operation on the same register.
> Moreover move write trace point in __mt7601u_vendor_single_wr
> 
> Signed-off-by: Lorenzo Bianconi 

Could you provide an example of which accesses make it problematic?
Is this fixing an actual bug?

Re: [PATCH] mt7601u: Fix system freeze after resuming from hibernation

[Jakub Kicinski]

On Thu, 15 Feb 2018 13:38:00 +0200, cantabile wrote:
> On 15/02/18 02:45, Jakub Kicinski wrote:
> > On Wed, 14 Feb 2018 13:34:38 +0200, cantabile wrote:  
> >> The firmware running on the device sometimes survives a reboot
> >> (firmware_running returns 1). When this happens the driver never calls
> >> request_firmware, which means the kernel's firmware handling code
> >> doesn't know this firmware should be cached before hibernating. Upon
> >> resuming from several hours of hibernation, the firmware is no longer
> >> running on the device, so the driver calls request_firmware. Since the
> >> firmware was never cached, it needs to be loaded from disk, and this is
> >> when the system freezes, somewhere in the xfs driver. Fix this by always
> >> requesting the firmware, whether it's already running on the device or not.
> >>
> >> Signed-off-by: John Smith <cantabile.d...@gmail.com>  
> > 
> > Thanks for tracking this down, but this seems like the wrong
> > direction.
> > 
> > What's your hard drive?  Is it some complex configuration which
> > prevents the block device from coming online after resume?
> > 
> > If it's really because of some peculiarities of XFS the fix should
> > go there, no driver will be able to load FW on resume...
> >   
> 
> My hard drive is a SATA SSD, with a regular MS-DOS partition table, with 
> three primary partitions: one ext4 mounted at /boot, one xfs mounted at 
> /, one xfs mounted at /home. No encryption, no software RAID, no logical 
> volumes, etc.
> 
> The kernel has a firmware caching mechanism to make sure that no driver 
> needs to load firmware from disk during the resume process. Because 
> that's unreliable. See this bit of the documentation: [1]
> 
> This is how it works: when the driver's probe callback calls 
> request_firmware, that function adds the firmware file's name in a 
> devres in your 'struct device'. When you suspend the system, the kernel 
> calls fw_pm_notify [2], which goes through all the 'struct device's and 
> looks for firmware file names previously added by request_firmware, and 
> loads into memory all the firmware files it finds. When you resume the 
> system, all calls to request_firmware ought to find the firmware files 
> already loaded in memory. After resuming, the kernel calls fw_pm_notify 
> again to release the cached firmware files (with some delay).
> 
> This caching mechanism currently doesn't always work for your driver 
> because your driver doesn't always call request_firmware from the probe 
> callback. Because the firmware running on the device sometimes survives 
> a reboot.

Thanks for the info.  Would it be cleaner to EXPORT fw_add_devm_name()
and just call that in case driver sees FW is already loaded?  That
should inform the fw subsystem that we want the image around in case of
hibernation, but there is no need to load it immediately?

> You can get some very useful messages if you compile firmware_class.c 
> with -DDEBUG [3]. This is how I figured out the problem.
> 
> [1] 
> https://www.kernel.org/doc/html/v4.13/driver-api/firmware/request_firmware.html#considerations-for-suspend-and-resume
> [2] 
> https://github.com/torvalds/linux/blob/master/drivers/base/firmware_class.c#L1804
> [3] https://www.kernel.org/doc/local/pr_debug.txt

Re: [PATCH] mt7601u: Fix system freeze after resuming from hibernation

[Jakub Kicinski]

On Wed, 14 Feb 2018 13:34:38 +0200, cantabile wrote:
> The firmware running on the device sometimes survives a reboot 
> (firmware_running returns 1). When this happens the driver never calls 
> request_firmware, which means the kernel's firmware handling code 
> doesn't know this firmware should be cached before hibernating. Upon 
> resuming from several hours of hibernation, the firmware is no longer 
> running on the device, so the driver calls request_firmware. Since the 
> firmware was never cached, it needs to be loaded from disk, and this is 
> when the system freezes, somewhere in the xfs driver. Fix this by always 
> requesting the firmware, whether it's already running on the device or not.
> 
> Signed-off-by: John Smith 

Thanks for tracking this down, but this seems like the wrong
direction.

What's your hard drive?  Is it some complex configuration which
prevents the block device from coming online after resume?  

If it's really because of some peculiarities of XFS the fix should 
go there, no driver will be able to load FW on resume...

> --- a/drivers/net/wireless/mediatek/mt7601u/mcu.c
> +++ b/drivers/net/wireless/mediatek/mt7601u/mcu.c
> @@ -420,13 +420,15 @@
>   mt7601u_wr(dev, MT_USB_DMA_CFG, (MT_USB_DMA_CFG_RX_BULK_EN |
>MT_USB_DMA_CFG_TX_BULK_EN));
> 
> - if (firmware_running(dev))
> - return 0;
> -
>   ret = request_firmware(, MT7601U_FIRMWARE, dev->dev);
>   if (ret)
>   return ret;
> 
> + if (firmware_running(dev)) {
> + release_firmware(fw);
> + return 0;
> + }
> +
>   if (!fw || !fw->data || fw->size < sizeof(*hdr))
>   goto err_inv_fw;
> 

Re: [PATCH 2/2] mt7601u: set device mac address in mt7601u_add_interface()

[Jakub Kicinski]

On Fri, 9 Feb 2018 10:49:21 +0100, Lorenzo Bianconi wrote:
> On Feb 08, Jakub Kicinski wrote:
> > On Thu,  8 Feb 2018 23:08:09 +0100, Lorenzo Bianconi wrote:  
> > > If mac80211 adds a vif with a different mac address respect to
> > > the eeprom one, the device will not be able to connect to the ap
> > > since the hw address has not been updated.
> > > Fix the issue updating hw mac address in mt7601u_add_interface routine
> > > 
> > > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1516935
> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
> > > ---
> > >  drivers/net/wireless/mediatek/mt7601u/main.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/net/wireless/mediatek/mt7601u/main.c 
> > > b/drivers/net/wireless/mediatek/mt7601u/main.c
> > > index 43ebd460ba86..3c9ea40d9584 100644
> > > --- a/drivers/net/wireless/mediatek/mt7601u/main.c
> > > +++ b/drivers/net/wireless/mediatek/mt7601u/main.c
> > > @@ -64,6 +64,9 @@ static int mt7601u_add_interface(struct ieee80211_hw 
> > > *hw,
> > >*/
> > >   mvif->idx = idx;
> > >  
> > > + if (!ether_addr_equal(dev->macaddr, vif->addr))
> > > + mt7601u_set_macaddr(dev, vif->addr);
> > > +
> > >   if (dev->wcid_mask[wcid / BITS_PER_LONG] & BIT(wcid % BITS_PER_LONG))
> > >   return -ENOSPC;
> > >   dev->wcid_mask[wcid / BITS_PER_LONG] |= BIT(wcid % BITS_PER_LONG);  
> > 
> > Sorry, my recollection of mac80211 code is waning, but can't we have
> > more than one vif as long as they are on the same channel?  
> 
> Hi Jakub,
> 
> yep, you are right, but according to my understanding (please correct me if
> it is wrong) current implementation supports just one interface in sta mode
> (i.e. mvif->idx is always 0) so my patchset just fixes the issue highlighted
> in the bugzilla since I do not know if the hw supports multiple concurrent 
> vifs
> in client mode. If so, I can extend the support to multiple client vifs if it 
> is
> a way to properly configure the rx filters to allow reception from multiple 
> mac
> addresses.

Ah, you're probably right, perhaps we would have one vif but multiple
stas.

Re: [PATCH 0/2] mt7601u: update mac addr if different from eeprom one

[Jakub Kicinski]

On Thu,  8 Feb 2018 23:08:07 +0100, Lorenzo Bianconi wrote:
> Lorenzo Bianconi (2):
>   mt7601u: move mt7601u_set_macaddr in mac related code
>   mt7601u: set device mac address in mt7601u_add_interface()
> 
>  drivers/net/wireless/mediatek/mt7601u/eeprom.c | 24 ++--
>  drivers/net/wireless/mediatek/mt7601u/mac.c| 16 
>  drivers/net/wireless/mediatek/mt7601u/mac.h|  1 +
>  drivers/net/wireless/mediatek/mt7601u/main.c   |  3 +++
>  4 files changed, 22 insertions(+), 22 deletions(-)
> 

Acked-by: Jakub Kicinski <kubak...@wp.pl>

Re: [PATCH 2/2] mt7601u: set device mac address in mt7601u_add_interface()

[Jakub Kicinski]

On Thu,  8 Feb 2018 23:08:09 +0100, Lorenzo Bianconi wrote:
> If mac80211 adds a vif with a different mac address respect to
> the eeprom one, the device will not be able to connect to the ap
> since the hw address has not been updated.
> Fix the issue updating hw mac address in mt7601u_add_interface routine
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1516935
> Signed-off-by: Lorenzo Bianconi 
> ---
>  drivers/net/wireless/mediatek/mt7601u/main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/wireless/mediatek/mt7601u/main.c 
> b/drivers/net/wireless/mediatek/mt7601u/main.c
> index 43ebd460ba86..3c9ea40d9584 100644
> --- a/drivers/net/wireless/mediatek/mt7601u/main.c
> +++ b/drivers/net/wireless/mediatek/mt7601u/main.c
> @@ -64,6 +64,9 @@ static int mt7601u_add_interface(struct ieee80211_hw *hw,
>*/
>   mvif->idx = idx;
>  
> + if (!ether_addr_equal(dev->macaddr, vif->addr))
> + mt7601u_set_macaddr(dev, vif->addr);
> +
>   if (dev->wcid_mask[wcid / BITS_PER_LONG] & BIT(wcid % BITS_PER_LONG))
>   return -ENOSPC;
>   dev->wcid_mask[wcid / BITS_PER_LONG] |= BIT(wcid % BITS_PER_LONG);

Sorry, my recollection of mac80211 code is waning, but can't we have
more than one vif as long as they are on the same channel?

Re: [PATCH 1/3] ath9k: remove stray backslash in Makefile

[Jakub Kicinski]

On Mon, 27 Nov 2017 18:56:21 +0100, Matthias Schiffer wrote:
> Signed-off-by: Matthias Schiffer 
> ---
>  drivers/net/wireless/ath/ath9k/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/Makefile 
> b/drivers/net/wireless/ath/ath9k/Makefile
> index 36a40ffdce15..90e4a341076c 100644
> --- a/drivers/net/wireless/ath/ath9k/Makefile
> +++ b/drivers/net/wireless/ath/ath9k/Makefile
> @@ -59,7 +59,7 @@ obj-$(CONFIG_ATH9K_HW) += ath9k_hw.o
>  obj-$(CONFIG_ATH9K_COMMON) += ath9k_common.o
>  ath9k_common-y:= common.o \
>   common-init.o \
> - common-beacon.o \
> + common-beacon.o
>  

It's not necessarily stray, there is nothing on the next line so it's
OK, and if you add \ at the end of all lines, you don't have to touch
the last line every time you add/remove something.  Sort of like
putting a , after last enum value.

Just my $.02, not that I mind either way :)

>  ath9k_common-$(CONFIG_ATH9K_COMMON_DEBUG) += common-debug.o \
>common-spectral.o

Re: [PATCH v2] mt7601u: check memory allocation failure

[Jakub Kicinski]

On Tue, 22 Aug 2017 00:06:17 +0200, Christophe JAILLET wrote:
> Check memory allocation failure and return -ENOMEM in such a case, as
> already done a few lines below.
> 
> As 'dev->tx_q' can be NULL, we also need to check for that in
> 'mt7601u_free_tx()', and return early.
> 
> Signed-off-by: Christophe JAILLET <christophe.jail...@wanadoo.fr>

Acked-by: Jakub Kicinski <kubak...@wp.pl>

Re: [PATCH] mt7601u: check memory allocation failure

[Jakub Kicinski]

On Mon, 21 Aug 2017 14:34:30 -0700, Jakub Kicinski wrote:
> On Mon, 21 Aug 2017 22:59:56 +0200, Christophe JAILLET wrote:
> > Check memory allocation failure and return -ENOMEM in such a case, as
> > already done a few lines below
> > 
> > Signed-off-by: Christophe JAILLET <christophe.jail...@wanadoo.fr>  
> 
> Acked-by: Jakub Kicinski <kubak...@wp.pl>

Wait, I take that back.  This code is a bit weird.  We would return an
error, then mt7601u_dma_init() will call mt7601u_free_tx_queue() which
doesn't check for tx_q == NULL condition.

Looks like mt7601u_free_tx() has to check for dev->tx_q == NULL and
return early if that's the case.  Or mt7601u_alloc_tx() should really
clean things up on it's own on failure.  Ugh.

Re: [PATCH] mt7601u: check memory allocation failure

[Jakub Kicinski]

On Mon, 21 Aug 2017 22:59:56 +0200, Christophe JAILLET wrote:
> Check memory allocation failure and return -ENOMEM in such a case, as
> already done a few lines below
> 
> Signed-off-by: Christophe JAILLET <christophe.jail...@wanadoo.fr>

Acked-by: Jakub Kicinski <kubak...@wp.pl>

Thanks!

Re: 'skb' buffer address information leakage

[Jakub Kicinski]

On Tue, 4 Jul 2017 13:12:18 +0800, Dison River wrote:
> drivers/net/ethernet/netronome/nfp/nfp_net_debugfs.c:167
>  seq_printf(file, " frag=%p", skb);

FWIW that's actually not a skb pointer.  The structure is defined like
this:

struct nfp_net_tx_buf {
union { 
struct sk_buff *skb;
void *frag;
};
dma_addr_t dma_addr;
short int fidx;
u16 pkt_cnt;
u32 real_len;
};

So the line in question is actually reading the frag pointer, I just
reused the skb variable, because this has to be read via READ_ONCE()
and NULL-checked so I thought that doing it separately for skb and
frag is a waste of LOC especially in debug code.  I will queue up a
clean up for after the merge window.

Thanks!

Re: [PATCH] mt7601u: wait for clear rxq when stopping mac

[Jakub Kicinski]

On Fri, 25 Nov 2016 03:13:34 -0800, Anthony Romano wrote:
> mt7601u_mac_stop_hw should stop polling the rxq once it remains empty
> but instead continues polling after the rxq status stays clear; bringing
> down the interface takes about six seconds from this alone.
> 
> Speed up path by exiting rxq loop once status repeatedly polls empty.
> 
> Signed-off-by: Anthony Romano <anthony.rom...@coreos.com>

Looks correct, the condition was inverted and we don't have to
msleep() when we see values go to zero.  Thanks!

Reviewed-by: Jakub Kicinski <kubak...@wp.pl>

[PATCHv8 0/4] register-field manipulation macros

[Jakub Kicinski]

Hi!

Small improvement suggested by Daniel Borkmann - use
two underscore prefix.

https://www.mail-archive.com/netdev@vger.kernel.org/msg125423.html

-- v6 blurb:

This set moves to a global header file macros which I find
very useful and worth popularising.  The basic problem is
that since C bitfields are not very dependable accessing
subfields of registers becomes slightly inconvenient.
It is nice to have the necessary mask and shift operations
wrapped in a macro and have that macro compute shift
at compilation time using FFS.

My implementation follows what Felix Fietkau has done in
mt76.  Hannes Frederic Sowa suggested more use of standard
Linux/GCC functions.  Since the RFC I've also added a 
compile-time check to validate that the value passed to
setters fits in the mask.

I attempted the use of static inlines instead of macros
but it makes GCC < 6.0 barf at the BUILD_BUG_ON()s.
I also noticed that forcing arguments to be u32 for inlines
makes the compiler use 32bit arithmetic where it could
get away with 64bit before (on 64bit machines, obviously).
That's a potential performance concern but probably not
a very practical one today.  Apart from looking "cleaner"
static inlines would have the advantage that we could #undef
the auxiliary macros at the end of the header.

Build bot caught a build failure with -Os set.  AFAICT gcc
did not handle temporary variable I put in the macro
expression too well.  I work around that by defining
__BUILD_BUG_ON_NOT_POWER_OF_2 and using it instead of
BUILD_BUG_ON(!tmp || is_power_of_2(tmp)).

I'm planning to use those macros in another driver soon,
Felix may also use them when upstreaming mt76 if he chooses
to do so.

IMHO if accepted it would be best to push these through
Kalle's wireless drivers' tree, since the only existing
user is in drivers/net/wireless/.

v8:
 - use two underscores for auxiliary macros (Daniel B).
v7:
 - drop the explicit type marking (u32/u64) - depend on the type
   of the mask instead;
 - only allow compilation time constant masks;
 - barf at "type of register too small to ever match mask" on get;
 - rename PUT -> PREP.
v6:
 - do a full rename in patch 2;
 - CC many people.
v5:
 - repost.
v4:
 - add documentation in the header.
v3:
 - don't use variables in statement expressions;
 - use __BUILD_BUG_ON_NOT_POWER_OF_2.
v2:
 - change Felix's email address.

Jakub Kicinski (4):
  add basic register-field manipulation macros
  mt7601u: remove redefinition of GENMASK
  mt7601u: remove unnecessary include
  mt7601u: use linux/bitfield.h

 drivers/net/wireless/mediatek/mt7601u/dma.c |  2 +-
 drivers/net/wireless/mediatek/mt7601u/dma.h | 10 ++-
 drivers/net/wireless/mediatek/mt7601u/eeprom.c  | 12 ++--
 drivers/net/wireless/mediatek/mt7601u/init.c| 10 +--
 drivers/net/wireless/mediatek/mt7601u/mac.c | 38 +-
 drivers/net/wireless/mediatek/mt7601u/main.c|  1 -
 drivers/net/wireless/mediatek/mt7601u/mcu.c | 20 +++---
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  4 +-
 drivers/net/wireless/mediatek/mt7601u/phy.c | 44 ++--
 drivers/net/wireless/mediatek/mt7601u/regs.h|  4 --
 drivers/net/wireless/mediatek/mt7601u/tx.c  | 19 ++---
 drivers/net/wireless/mediatek/mt7601u/util.h| 77 
 include/linux/bitfield.h| 93 +
 include/linux/bug.h |  3 +
 14 files changed, 177 insertions(+), 160 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h
 create mode 100644 include/linux/bitfield.h

-- 
1.9.1

[PATCHv8 4/4] mt7601u: use linux/bitfield.h

[Jakub Kicinski]

Use the newly added linux/bitfield.h.

Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
Reviewed-by: Dinan Gunawardena <dinan.gunaward...@netronome.com>
---
 drivers/net/wireless/mediatek/mt7601u/dma.c |  2 +-
 drivers/net/wireless/mediatek/mt7601u/dma.h | 10 ++--
 drivers/net/wireless/mediatek/mt7601u/eeprom.c  | 12 ++--
 drivers/net/wireless/mediatek/mt7601u/init.c| 10 ++--
 drivers/net/wireless/mediatek/mt7601u/mac.c | 38 ++--
 drivers/net/wireless/mediatek/mt7601u/mcu.c | 20 +++
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  4 +-
 drivers/net/wireless/mediatek/mt7601u/phy.c | 44 +++---
 drivers/net/wireless/mediatek/mt7601u/tx.c  | 19 +++---
 drivers/net/wireless/mediatek/mt7601u/util.h| 77 -
 10 files changed, 81 insertions(+), 155 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h

diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.c 
b/drivers/net/wireless/mediatek/mt7601u/dma.c
index 57a80cfa39b1..a8bc064bc14f 100644
--- a/drivers/net/wireless/mediatek/mt7601u/dma.c
+++ b/drivers/net/wireless/mediatek/mt7601u/dma.c
@@ -103,7 +103,7 @@ static void mt7601u_rx_process_seg(struct mt7601u_dev *dev, 
u8 *data,
 
if (unlikely(rxwi->zero[0] || rxwi->zero[1] || rxwi->zero[2]))
dev_err_once(dev->dev, "Error: RXWI zero fields are set\n");
-   if (unlikely(MT76_GET(MT_RXD_INFO_TYPE, fce_info)))
+   if (unlikely(FIELD_GET(MT_RXD_INFO_TYPE, fce_info)))
dev_err_once(dev->dev, "Error: RX path seen a non-pkt urb\n");
 
trace_mt_rx(dev, rxwi, fce_info);
diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.h 
b/drivers/net/wireless/mediatek/mt7601u/dma.h
index 978e8a90b87f..270d126880c0 100644
--- a/drivers/net/wireless/mediatek/mt7601u/dma.h
+++ b/drivers/net/wireless/mediatek/mt7601u/dma.h
@@ -18,8 +18,6 @@
 #include 
 #include 
 
-#include "util.h"
-
 #define MT_DMA_HDR_LEN 4
 #define MT_RX_INFO_LEN 4
 #define MT_FCE_INFO_LEN4
@@ -79,9 +77,9 @@ static inline int mt7601u_dma_skb_wrap(struct sk_buff *skb,
 */
 
info = flags |
-   MT76_SET(MT_TXD_INFO_LEN, round_up(skb->len, 4)) |
-   MT76_SET(MT_TXD_INFO_D_PORT, d_port) |
-   MT76_SET(MT_TXD_INFO_TYPE, type);
+   FIELD_PREP(MT_TXD_INFO_LEN, round_up(skb->len, 4)) |
+   FIELD_PREP(MT_TXD_INFO_D_PORT, d_port) |
+   FIELD_PREP(MT_TXD_INFO_TYPE, type);
 
put_unaligned_le32(info, skb_push(skb, sizeof(info)));
return skb_put_padto(skb, round_up(skb->len, 4) + 4);
@@ -90,7 +88,7 @@ static inline int mt7601u_dma_skb_wrap(struct sk_buff *skb,
 static inline int
 mt7601u_dma_skb_wrap_pkt(struct sk_buff *skb, enum mt76_qsel qsel, u32 flags)
 {
-   flags |= MT76_SET(MT_TXD_PKT_INFO_QSEL, qsel);
+   flags |= FIELD_PREP(MT_TXD_PKT_INFO_QSEL, qsel);
return mt7601u_dma_skb_wrap(skb, WLAN_PORT, DMA_PACKET, flags);
 }
 
diff --git a/drivers/net/wireless/mediatek/mt7601u/eeprom.c 
b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
index 8d8ee0344f7b..da6faea092d6 100644
--- a/drivers/net/wireless/mediatek/mt7601u/eeprom.c
+++ b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
@@ -45,8 +45,8 @@ mt7601u_efuse_read(struct mt7601u_dev *dev, u16 addr, u8 
*data,
val = mt76_rr(dev, MT_EFUSE_CTRL);
val &= ~(MT_EFUSE_CTRL_AIN |
 MT_EFUSE_CTRL_MODE);
-   val |= MT76_SET(MT_EFUSE_CTRL_AIN, addr & ~0xf) |
-  MT76_SET(MT_EFUSE_CTRL_MODE, mode) |
+   val |= FIELD_PREP(MT_EFUSE_CTRL_AIN, addr & ~0xf) |
+  FIELD_PREP(MT_EFUSE_CTRL_MODE, mode) |
   MT_EFUSE_CTRL_KICK;
mt76_wr(dev, MT_EFUSE_CTRL, val);
 
@@ -128,8 +128,8 @@ mt7601u_set_chip_cap(struct mt7601u_dev *dev, u8 *eeprom)
if (!field_valid(nic_conf0 >> 8))
return;
 
-   if (MT76_GET(MT_EE_NIC_CONF_0_RX_PATH, nic_conf0) > 1 ||
-   MT76_GET(MT_EE_NIC_CONF_0_TX_PATH, nic_conf0) > 1)
+   if (FIELD_GET(MT_EE_NIC_CONF_0_RX_PATH, nic_conf0) > 1 ||
+   FIELD_GET(MT_EE_NIC_CONF_0_TX_PATH, nic_conf0) > 1)
dev_err(dev->dev,
"Error: device has more than 1 RX/TX stream!\n");
 }
@@ -150,7 +150,7 @@ mt7601u_set_macaddr(struct mt7601u_dev *dev, const u8 
*eeprom)
 
mt76_wr(dev, MT_MAC_ADDR_DW0, get_unaligned_le32(dev->macaddr));
mt76_wr(dev, MT_MAC_ADDR_DW1, get_unaligned_le16(dev->macaddr + 4) |
-   MT76_SET(MT_MAC_ADDR_DW1_U2ME_MASK, 0xff));
+   FIELD_PREP(MT_MAC_ADDR_DW1_U2ME_MASK, 0xff));
 
return 0;
 }
@@ -176,7 +176,7 @@ mt7601u_set_channel_power(struct mt7601u_dev *dev, u8 
*eeprom)
u8 max_pwr;
 
val = mt7601u_rr(dev, MT_TX_ALC_CFG_0);

[PATCHv8 3/4] mt7601u: remove unnecessary include

[Jakub Kicinski]

There is no need to include linux/version.h in a in-tree
driver.

Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
Reviewed-by: Dinan Gunawardena <dinan.gunaward...@netronome.com>
---
 drivers/net/wireless/mediatek/mt7601u/main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt7601u/main.c 
b/drivers/net/wireless/mediatek/mt7601u/main.c
index e70dd9523911..43ebd460ba86 100644
--- a/drivers/net/wireless/mediatek/mt7601u/main.c
+++ b/drivers/net/wireless/mediatek/mt7601u/main.c
@@ -15,7 +15,6 @@
 #include "mt7601u.h"
 #include "mac.h"
 #include 
-#include 
 
 static int mt7601u_start(struct ieee80211_hw *hw)
 {
-- 
1.9.1

[PATCHv8 2/4] mt7601u: remove redefinition of GENMASK

[Jakub Kicinski]

Remove redefinition of GENMASK which should not be there
in the upstream version of the code.

Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
Reviewed-by: Dinan Gunawardena <dinan.gunaward...@netronome.com>
---
 drivers/net/wireless/mediatek/mt7601u/regs.h | 4 
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt7601u/regs.h 
b/drivers/net/wireless/mediatek/mt7601u/regs.h
index afd8978e83fa..27a429d90cec 100644
--- a/drivers/net/wireless/mediatek/mt7601u/regs.h
+++ b/drivers/net/wireless/mediatek/mt7601u/regs.h
@@ -17,10 +17,6 @@
 
 #include 
 
-#ifndef GENMASK
-#define GENMASK(h, l)   (((U32_C(1) << ((h) - (l) + 1)) - 1) << (l))
-#endif
-
 #define MT_ASIC_VERSION0x
 
 #define MT76XX_REV_E3  0x22
-- 
1.9.1

[PATCHv8 1/4] add basic register-field manipulation macros

[Jakub Kicinski]

Common approach to accessing register fields is to define
structures or sets of macros containing mask and shift pair.
Operations on the register are then performed as follows:

 field = (reg >> shift) & mask;

 reg &= ~(mask << shift);
 reg |= (field & mask) << shift;

Defining shift and mask separately is tedious.  Ivo van Doorn
came up with an idea of computing them at compilation time
based on a single shifted mask (later refined by Felix) which
can be used like this:

 #define REG_FIELD 0x000ff000

 field = FIELD_GET(REG_FIELD, reg);

 reg &= ~REG_FIELD;
 reg |= FIELD_PREP(REG_FIELD, field);

FIELD_{GET,PREP} macros take care of finding out what the
appropriate shift is based on compilation time ffs operation.

GENMASK can be used to define registers (which is usually
less error-prone and easier to match with datasheets).

This approach is the most convenient I've seen so to limit code
multiplication let's move the macros to a global header file.
Attempts to use static inlines instead of macros failed due
to false positive triggering of BUILD_BUG_ON()s, especially with
GCC < 6.0.

Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
Reviewed-by: Dinan Gunawardena <dinan.gunaward...@netronome.com>
---
 v8:
  - use two underscores as prefix for __bf_shf() and 
__BF_FIELD_CHECK()

 include/linux/bitfield.h | 93 
 include/linux/bug.h  |  3 ++
 2 files changed, 96 insertions(+)
 create mode 100644 include/linux/bitfield.h

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
new file mode 100644
index ..f6505d83069d
--- /dev/null
+++ b/include/linux/bitfield.h
@@ -0,0 +1,93 @@
+/*
+ * Copyright (C) 2014 Felix Fietkau <n...@nbd.name>
+ * Copyright (C) 2004 - 2009 Ivo van Doorn <ivdo...@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _LINUX_BITFIELD_H
+#define _LINUX_BITFIELD_H
+
+#include 
+
+/*
+ * Bitfield access macros
+ *
+ * FIELD_{GET,PREP} macros take as first parameter shifted mask
+ * from which they extract the base mask and shift amount.
+ * Mask must be a compilation time constant.
+ *
+ * Example:
+ *
+ *  #define REG_FIELD_A  GENMASK(6, 0)
+ *  #define REG_FIELD_B  BIT(7)
+ *  #define REG_FIELD_C  GENMASK(15, 8)
+ *  #define REG_FIELD_D  GENMASK(31, 16)
+ *
+ * Get:
+ *  a = FIELD_GET(REG_FIELD_A, reg);
+ *  b = FIELD_GET(REG_FIELD_B, reg);
+ *
+ * Set:
+ *  reg = FIELD_PREP(REG_FIELD_A, 1) |
+ *   FIELD_PREP(REG_FIELD_B, 0) |
+ *   FIELD_PREP(REG_FIELD_C, c) |
+ *   FIELD_PREP(REG_FIELD_D, 0x40);
+ *
+ * Modify:
+ *  reg &= ~REG_FIELD_C;
+ *  reg |= FIELD_PREP(REG_FIELD_C, c);
+ */
+
+#define __bf_shf(x) (__builtin_ffsll(x) - 1)
+
+#define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx)  \
+   ({  \
+   BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),  \
+_pfx "mask is not constant");  \
+   BUILD_BUG_ON_MSG(!(_mask), _pfx "mask is zero");\
+   BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?   \
+~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
+_pfx "value too large for the field"); \
+   BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \
+_pfx "type of reg too small for mask"); \
+   __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
+ (1ULL << __bf_shf(_mask))); \
+   })
+
+/**
+ * FIELD_PREP() - prepare a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_val:  value to put in the field
+ *
+ * FIELD_PREP() masks and shifts up the value.  The result should
+ * be combined with other fields of the bitfield using logical OR.
+ */
+#define FIELD_PREP(_mask, _val)
\
+   ({  \
+   __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");\
+   ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask);   \
+   })
+
+/**
+ * FIELD_GET() - extract a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_reg:  32bit value of entire bitfield
+ *
+ * FIELD_GET() extr

Re: [PATCHv7 4/4] mt7601u: use linux/bitfield.h

[Jakub Kicinski]

On Mon, 22 Aug 2016 21:19:41 +0200, Arend Van Spriel wrote:
> On 22-8-2016 12:52, Jakub Kicinski wrote:
> > On Fri, 19 Aug 2016 21:02:02 +0200, Arend Van Spriel wrote:  
> >> On 19-8-2016 18:44, Jakub Kicinski wrote:  
> >>> Use the newly added linux/bitfield.h.
> >>>
> >>> Reviewed-by: Dinan Gunawardena <dinan.gunaward...@netronome.com>
> >>> Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
> >>> ---  
> 
> [snip]
> 
> >>> diff --git a/drivers/net/wireless/mediatek/mt7601u/eeprom.c 
> >>> b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
> >>> index 8d8ee0344f7b..da6faea092d6 100644
> >>> --- a/drivers/net/wireless/mediatek/mt7601u/eeprom.c
> >>> +++ b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
> >>> @@ -45,8 +45,8 @@ mt7601u_efuse_read(struct mt7601u_dev *dev, u16 addr, 
> >>> u8 *data,
> >>>   val = mt76_rr(dev, MT_EFUSE_CTRL);
> >>>   val &= ~(MT_EFUSE_CTRL_AIN |
> >>>MT_EFUSE_CTRL_MODE);
> >>> - val |= MT76_SET(MT_EFUSE_CTRL_AIN, addr & ~0xf) |
> >>> -MT76_SET(MT_EFUSE_CTRL_MODE, mode) |
> >>> + val |= FIELD_PREP(MT_EFUSE_CTRL_AIN, addr & ~0xf) |
> >>> +FIELD_PREP(MT_EFUSE_CTRL_MODE, mode) |
> >>>  MT_EFUSE_CTRL_KICK;
> >>
> >> MT_EFUSE_CTRL_KICK is probably a 1-bit field in MT_EFUSE_CTRL register.
> >> It looks like you did not want to go all the way although you do give an
> >> example in bitfield.h, ie. + *  #define REG_FIELD_B  BIT(7).  
> > 
> > True, I just wanted to show in the examples that BIT() is OK to use.
> > My feeling is that ORing in always set flags is acceptable, or at least
> > it was my feeling when I wrote this code ;)  
> 
> I find it tricky to have the user do the field clearing separately.
> Especially if you allow the calling function to pass additional opaque
> "flags". In my experience this is more error prone than anything else
> when dealing with bit fields and as such it is unfortunate this aspect
> is not addressed in your patches.

I don't think anything in this set prevents people from adding a
FIELD_SET() wrapper.  Quite the opposite and I'd encourage it.

> I would rather see:
> 
>   val = mt76_rr(dev, MT_EFUSE_CTRL);
>   FIELD_SET(val, MT_EFUSE_CTRL_AIN, addr & ~0xf);
>   FIELD_SET(val, MT_EFUSE_CTRL_MODE, mode);
>   FIELD_SET(val, MT_EFUSE_CTRL_KICK, 1);
>   mt76_wr(dev, MT_EFUSE_CTRL, val);
> 
> in which FIELD_SET takes care of clearing the indicated field.

Yes, I agree this is a good solution as well.  The reason I didn't go
with this approach (apart from modifying an argument of a macro ;)) is
the experience with rt2x00 which followed this design and I wasn't
particularly fond of the resulting code.

I find PREP macro easier to read (less parameters).  It's also
often convenient to not have to zero the variable for pure write
and be able to combine the values in a parameter list:

+   mt7601u_wr(dev, MT_TXOP_CTRL_CFG,
+  FIELD_PREP(MT_TXOP_TRUN_EN, 0x3f) |
+  FIELD_PREP(MT_TXOP_EXT_CCA_DLY, 0x58));
or:
+   reg = cpu_to_le32(FIELD_PREP(MT_TXD_INFO_TYPE, DMA_PACKET) |
+ FIELD_PREP(MT_TXD_INFO_D_PORT, CPU_TX_PORT) |
+ FIELD_PREP(MT_TXD_INFO_LEN, len));

So each approach has it's advantages and I think they complement each
other.

Please note that I'm just using mt7601u as an example, I really don't
need SET in the new code I'm trying to use these macros for now [1].

[1] https://patchwork.ozlabs.org/patch/628779/

Re: [PATCHv7 4/4] mt7601u: use linux/bitfield.h

[Jakub Kicinski]

On Fri, 19 Aug 2016 21:02:02 +0200, Arend Van Spriel wrote:
> On 19-8-2016 18:44, Jakub Kicinski wrote:
> > Use the newly added linux/bitfield.h.
> > 
> > Reviewed-by: Dinan Gunawardena <dinan.gunaward...@netronome.com>
> > Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
> > ---
> >  drivers/net/wireless/mediatek/mt7601u/dma.c |  2 +-
> >  drivers/net/wireless/mediatek/mt7601u/dma.h | 10 ++--
> >  drivers/net/wireless/mediatek/mt7601u/eeprom.c  | 12 ++--
> >  drivers/net/wireless/mediatek/mt7601u/init.c| 10 ++--
> >  drivers/net/wireless/mediatek/mt7601u/mac.c | 38 ++--
> >  drivers/net/wireless/mediatek/mt7601u/mcu.c | 20 +++
> >  drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  4 +-
> >  drivers/net/wireless/mediatek/mt7601u/phy.c | 44 +++---
> >  drivers/net/wireless/mediatek/mt7601u/tx.c  | 19 +++---
> >  drivers/net/wireless/mediatek/mt7601u/util.h| 77 
> > -
> >  10 files changed, 81 insertions(+), 155 deletions(-)
> >  delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h
> > 
> > diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.c 
> > b/drivers/net/wireless/mediatek/mt7601u/dma.c
> > index 57a80cfa39b1..a8bc064bc14f 100644
> > --- a/drivers/net/wireless/mediatek/mt7601u/dma.c
> > +++ b/drivers/net/wireless/mediatek/mt7601u/dma.c
> > @@ -103,7 +103,7 @@ static void mt7601u_rx_process_seg(struct mt7601u_dev 
> > *dev, u8 *data,
> >  
> > if (unlikely(rxwi->zero[0] || rxwi->zero[1] || rxwi->zero[2]))
> > dev_err_once(dev->dev, "Error: RXWI zero fields are set\n");
> > -   if (unlikely(MT76_GET(MT_RXD_INFO_TYPE, fce_info)))
> > +   if (unlikely(FIELD_GET(MT_RXD_INFO_TYPE, fce_info)))
> > dev_err_once(dev->dev, "Error: RX path seen a non-pkt urb\n");
> >  
> > trace_mt_rx(dev, rxwi, fce_info);
> > diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.h 
> > b/drivers/net/wireless/mediatek/mt7601u/dma.h
> > index 978e8a90b87f..270d126880c0 100644
> > --- a/drivers/net/wireless/mediatek/mt7601u/dma.h
> > +++ b/drivers/net/wireless/mediatek/mt7601u/dma.h
> > @@ -18,8 +18,6 @@
> >  #include 
> >  #include 
> >  
> > -#include "util.h"
> > -
> >  #define MT_DMA_HDR_LEN 4
> >  #define MT_RX_INFO_LEN 4
> >  #define MT_FCE_INFO_LEN4
> > @@ -79,9 +77,9 @@ static inline int mt7601u_dma_skb_wrap(struct sk_buff 
> > *skb,
> >  */
> >  
> > info = flags |
> > -   MT76_SET(MT_TXD_INFO_LEN, round_up(skb->len, 4)) |
> > -   MT76_SET(MT_TXD_INFO_D_PORT, d_port) |
> > -   MT76_SET(MT_TXD_INFO_TYPE, type);
> > +   FIELD_PREP(MT_TXD_INFO_LEN, round_up(skb->len, 4)) |
> > +   FIELD_PREP(MT_TXD_INFO_D_PORT, d_port) |
> > +   FIELD_PREP(MT_TXD_INFO_TYPE, type);  
> 
> So what are those flags? Is there no field definition for those.
> 
> > put_unaligned_le32(info, skb_push(skb, sizeof(info)));
> > return skb_put_padto(skb, round_up(skb->len, 4) + 4);
> > @@ -90,7 +88,7 @@ static inline int mt7601u_dma_skb_wrap(struct sk_buff 
> > *skb,
> >  static inline int
> >  mt7601u_dma_skb_wrap_pkt(struct sk_buff *skb, enum mt76_qsel qsel, u32 
> > flags)
> >  {
> > -   flags |= MT76_SET(MT_TXD_PKT_INFO_QSEL, qsel);
> > +   flags |= FIELD_PREP(MT_TXD_PKT_INFO_QSEL, qsel);  
> 
> Ah. This is the flags being ORed in above. I suppose there are more
> callsites to mt7601u_dma_skb_wrap().

Yes, the field layout differs slightly between data and MCU commands.
Additionally some packet flags depend on mac80211 info and I didn't
want to pollute dma.h with mac80211 info so I just pass those flags
as opaque u32.

> > return mt7601u_dma_skb_wrap(skb, WLAN_PORT, DMA_PACKET, flags);
> >  }
> >  
> > diff --git a/drivers/net/wireless/mediatek/mt7601u/eeprom.c 
> > b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
> > index 8d8ee0344f7b..da6faea092d6 100644
> > --- a/drivers/net/wireless/mediatek/mt7601u/eeprom.c
> > +++ b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
> > @@ -45,8 +45,8 @@ mt7601u_efuse_read(struct mt7601u_dev *dev, u16 addr, u8 
> > *data,
> > val = mt76_rr(dev, MT_EFUSE_CTRL);
> > val &= ~(MT_EFUSE_CTRL_AIN |
> >  MT_EFUSE_CTRL_MODE);
> > -   val |= MT76_SET(MT_EFUSE_CTRL_AIN, addr & ~0xf) |
> > -  MT76_SET(MT_EFUSE_CTRL_MODE, mode) |
> > +   val |= FIELD_PREP(MT_EFUSE_CTRL_AIN, addr & ~0xf) |
> > +  FIELD_PREP(MT_EFUSE_CTRL_MODE, mode) |
> >MT_EFUSE_CTRL_KICK;  
> 
> MT_EFUSE_CTRL_KICK is probably a 1-bit field in MT_EFUSE_CTRL register.
> It looks like you did not want to go all the way although you do give an
> example in bitfield.h, ie. + *  #define REG_FIELD_B  BIT(7).

True, I just wanted to show in the examples that BIT() is OK to use.
My feeling is that ORing in always set flags is acceptable, or at least
it was my feeling when I wrote this code ;)

[PATCHv7 1/4] add basic register-field manipulation macros

[Jakub Kicinski]

Common approach to accessing register fields is to define
structures or sets of macros containing mask and shift pair.
Operations on the register are then performed as follows:

 field = (reg >> shift) & mask;

 reg &= ~(mask << shift);
 reg |= (field & mask) << shift;

Defining shift and mask separately is tedious.  Ivo van Doorn
came up with an idea of computing them at compilation time
based on a single shifted mask (later refined by Felix) which
can be used like this:

 #define REG_FIELD 0x000ff000

 field = FIELD_GET(REG_FIELD, reg);

 reg &= ~REG_FIELD;
 reg |= FIELD_PREP(REG_FIELD, field);

FIELD_{GET,PREP} macros take care of finding out what the
appropriate shift is based on compilation time ffs operation.

GENMASK can be used to define registers (which is usually
less error-prone and easier to match with datasheets).

This approach is the most convenient I've seen so to limit code
multiplication let's move the macros to a global header file.
Attempts to use static inlines instead of macros failed due
to false positive triggering of BUILD_BUG_ON()s, especially with
GCC < 6.0.

Reviewed-by: Dinan Gunawardena <dinan.gunaward...@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
---
 include/linux/bitfield.h | 93 
 include/linux/bug.h  |  3 ++
 2 files changed, 96 insertions(+)
 create mode 100644 include/linux/bitfield.h

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
new file mode 100644
index ..32ca8863e66d
--- /dev/null
+++ b/include/linux/bitfield.h
@@ -0,0 +1,93 @@
+/*
+ * Copyright (C) 2014 Felix Fietkau <n...@nbd.name>
+ * Copyright (C) 2004 - 2009 Ivo van Doorn <ivdo...@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _LINUX_BITFIELD_H
+#define _LINUX_BITFIELD_H
+
+#include 
+
+/*
+ * Bitfield access macros
+ *
+ * FIELD_{GET,PREP} macros take as first parameter shifted mask
+ * from which they extract the base mask and shift amount.
+ * Mask must be a compilation time constant.
+ *
+ * Example:
+ *
+ *  #define REG_FIELD_A  GENMASK(6, 0)
+ *  #define REG_FIELD_B  BIT(7)
+ *  #define REG_FIELD_C  GENMASK(15, 8)
+ *  #define REG_FIELD_D  GENMASK(31, 16)
+ *
+ * Get:
+ *  a = FIELD_GET(REG_FIELD_A, reg);
+ *  b = FIELD_GET(REG_FIELD_B, reg);
+ *
+ * Set:
+ *  reg = FIELD_PREP(REG_FIELD_A, 1) |
+ *   FIELD_PREP(REG_FIELD_B, 0) |
+ *   FIELD_PREP(REG_FIELD_C, c) |
+ *   FIELD_PREP(REG_FIELD_D, 0x40);
+ *
+ * Modify:
+ *  reg &= ~REG_FIELD_C;
+ *  reg |= FIELD_PREP(REG_FIELD_C, c);
+ */
+
+#define _bf_shf(x) (__builtin_ffsll(x) - 1)
+
+#define _BF_FIELD_CHECK(_mask, _reg, _val, _pfx)   \
+   ({  \
+   BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),  \
+_pfx "mask is not constant");  \
+   BUILD_BUG_ON_MSG(!(_mask), _pfx "mask is zero");\
+   BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?   \
+~((_mask) >> _bf_shf(_mask)) & (_val) : 0, \
+_pfx "value too large for the field"); \
+   BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \
+_pfx "type of reg too small for mask"); \
+   __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
+ (1ULL << _bf_shf(_mask))); \
+   })
+
+/**
+ * FIELD_PREP() - prepare a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_val:  value to put in the field
+ *
+ * FIELD_PREP() masks and shifts up the value.  The result should
+ * be combined with other fields of the bitfield using logical OR.
+ */
+#define FIELD_PREP(_mask, _val)
\
+   ({  \
+   _BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
+   ((typeof(_mask))(_val) << _bf_shf(_mask)) & (_mask);\
+   })
+
+/**
+ * FIELD_GET() - extract a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_reg:  32bit value of entire bitfield
+ *
+ * FIELD_GET() extr

[PATCHv7 3/4] mt7601u: remove unnecessary include

[Jakub Kicinski]

There is no need to include linux/version.h in a in-tree
driver.

Reviewed-by: Dinan Gunawardena <dinan.gunaward...@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
---
 drivers/net/wireless/mediatek/mt7601u/main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt7601u/main.c 
b/drivers/net/wireless/mediatek/mt7601u/main.c
index e70dd9523911..43ebd460ba86 100644
--- a/drivers/net/wireless/mediatek/mt7601u/main.c
+++ b/drivers/net/wireless/mediatek/mt7601u/main.c
@@ -15,7 +15,6 @@
 #include "mt7601u.h"
 #include "mac.h"
 #include 
-#include 
 
 static int mt7601u_start(struct ieee80211_hw *hw)
 {
-- 
1.9.1

[PATCHv7 2/4] mt7601u: remove redefinition of GENMASK

[Jakub Kicinski]

Remove redefinition of GENMASK which should not be there
in the upstream version of the code.

Reviewed-by: Dinan Gunawardena <dinan.gunaward...@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
---
 drivers/net/wireless/mediatek/mt7601u/regs.h | 4 
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt7601u/regs.h 
b/drivers/net/wireless/mediatek/mt7601u/regs.h
index afd8978e83fa..27a429d90cec 100644
--- a/drivers/net/wireless/mediatek/mt7601u/regs.h
+++ b/drivers/net/wireless/mediatek/mt7601u/regs.h
@@ -17,10 +17,6 @@
 
 #include 
 
-#ifndef GENMASK
-#define GENMASK(h, l)   (((U32_C(1) << ((h) - (l) + 1)) - 1) << (l))
-#endif
-
 #define MT_ASIC_VERSION0x
 
 #define MT76XX_REV_E3  0x22
-- 
1.9.1

[PATCHv7 4/4] mt7601u: use linux/bitfield.h

[Jakub Kicinski]

Use the newly added linux/bitfield.h.

Reviewed-by: Dinan Gunawardena <dinan.gunaward...@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
---
 drivers/net/wireless/mediatek/mt7601u/dma.c |  2 +-
 drivers/net/wireless/mediatek/mt7601u/dma.h | 10 ++--
 drivers/net/wireless/mediatek/mt7601u/eeprom.c  | 12 ++--
 drivers/net/wireless/mediatek/mt7601u/init.c| 10 ++--
 drivers/net/wireless/mediatek/mt7601u/mac.c | 38 ++--
 drivers/net/wireless/mediatek/mt7601u/mcu.c | 20 +++
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  4 +-
 drivers/net/wireless/mediatek/mt7601u/phy.c | 44 +++---
 drivers/net/wireless/mediatek/mt7601u/tx.c  | 19 +++---
 drivers/net/wireless/mediatek/mt7601u/util.h| 77 -
 10 files changed, 81 insertions(+), 155 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h

diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.c 
b/drivers/net/wireless/mediatek/mt7601u/dma.c
index 57a80cfa39b1..a8bc064bc14f 100644
--- a/drivers/net/wireless/mediatek/mt7601u/dma.c
+++ b/drivers/net/wireless/mediatek/mt7601u/dma.c
@@ -103,7 +103,7 @@ static void mt7601u_rx_process_seg(struct mt7601u_dev *dev, 
u8 *data,
 
if (unlikely(rxwi->zero[0] || rxwi->zero[1] || rxwi->zero[2]))
dev_err_once(dev->dev, "Error: RXWI zero fields are set\n");
-   if (unlikely(MT76_GET(MT_RXD_INFO_TYPE, fce_info)))
+   if (unlikely(FIELD_GET(MT_RXD_INFO_TYPE, fce_info)))
dev_err_once(dev->dev, "Error: RX path seen a non-pkt urb\n");
 
trace_mt_rx(dev, rxwi, fce_info);
diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.h 
b/drivers/net/wireless/mediatek/mt7601u/dma.h
index 978e8a90b87f..270d126880c0 100644
--- a/drivers/net/wireless/mediatek/mt7601u/dma.h
+++ b/drivers/net/wireless/mediatek/mt7601u/dma.h
@@ -18,8 +18,6 @@
 #include 
 #include 
 
-#include "util.h"
-
 #define MT_DMA_HDR_LEN 4
 #define MT_RX_INFO_LEN 4
 #define MT_FCE_INFO_LEN4
@@ -79,9 +77,9 @@ static inline int mt7601u_dma_skb_wrap(struct sk_buff *skb,
 */
 
info = flags |
-   MT76_SET(MT_TXD_INFO_LEN, round_up(skb->len, 4)) |
-   MT76_SET(MT_TXD_INFO_D_PORT, d_port) |
-   MT76_SET(MT_TXD_INFO_TYPE, type);
+   FIELD_PREP(MT_TXD_INFO_LEN, round_up(skb->len, 4)) |
+   FIELD_PREP(MT_TXD_INFO_D_PORT, d_port) |
+   FIELD_PREP(MT_TXD_INFO_TYPE, type);
 
put_unaligned_le32(info, skb_push(skb, sizeof(info)));
return skb_put_padto(skb, round_up(skb->len, 4) + 4);
@@ -90,7 +88,7 @@ static inline int mt7601u_dma_skb_wrap(struct sk_buff *skb,
 static inline int
 mt7601u_dma_skb_wrap_pkt(struct sk_buff *skb, enum mt76_qsel qsel, u32 flags)
 {
-   flags |= MT76_SET(MT_TXD_PKT_INFO_QSEL, qsel);
+   flags |= FIELD_PREP(MT_TXD_PKT_INFO_QSEL, qsel);
return mt7601u_dma_skb_wrap(skb, WLAN_PORT, DMA_PACKET, flags);
 }
 
diff --git a/drivers/net/wireless/mediatek/mt7601u/eeprom.c 
b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
index 8d8ee0344f7b..da6faea092d6 100644
--- a/drivers/net/wireless/mediatek/mt7601u/eeprom.c
+++ b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
@@ -45,8 +45,8 @@ mt7601u_efuse_read(struct mt7601u_dev *dev, u16 addr, u8 
*data,
val = mt76_rr(dev, MT_EFUSE_CTRL);
val &= ~(MT_EFUSE_CTRL_AIN |
 MT_EFUSE_CTRL_MODE);
-   val |= MT76_SET(MT_EFUSE_CTRL_AIN, addr & ~0xf) |
-  MT76_SET(MT_EFUSE_CTRL_MODE, mode) |
+   val |= FIELD_PREP(MT_EFUSE_CTRL_AIN, addr & ~0xf) |
+  FIELD_PREP(MT_EFUSE_CTRL_MODE, mode) |
   MT_EFUSE_CTRL_KICK;
mt76_wr(dev, MT_EFUSE_CTRL, val);
 
@@ -128,8 +128,8 @@ mt7601u_set_chip_cap(struct mt7601u_dev *dev, u8 *eeprom)
if (!field_valid(nic_conf0 >> 8))
return;
 
-   if (MT76_GET(MT_EE_NIC_CONF_0_RX_PATH, nic_conf0) > 1 ||
-   MT76_GET(MT_EE_NIC_CONF_0_TX_PATH, nic_conf0) > 1)
+   if (FIELD_GET(MT_EE_NIC_CONF_0_RX_PATH, nic_conf0) > 1 ||
+   FIELD_GET(MT_EE_NIC_CONF_0_TX_PATH, nic_conf0) > 1)
dev_err(dev->dev,
"Error: device has more than 1 RX/TX stream!\n");
 }
@@ -150,7 +150,7 @@ mt7601u_set_macaddr(struct mt7601u_dev *dev, const u8 
*eeprom)
 
mt76_wr(dev, MT_MAC_ADDR_DW0, get_unaligned_le32(dev->macaddr));
mt76_wr(dev, MT_MAC_ADDR_DW1, get_unaligned_le16(dev->macaddr + 4) |
-   MT76_SET(MT_MAC_ADDR_DW1_U2ME_MASK, 0xff));
+   FIELD_PREP(MT_MAC_ADDR_DW1_U2ME_MASK, 0xff));
 
return 0;
 }
@@ -176,7 +176,7 @@ mt7601u_set_channel_power(struct mt7601u_dev *dev, u8 
*eeprom)
u8 max_pwr;
 
val = mt7601u_rr(dev, MT_TX_ALC_CFG_0);

[PATCHv7 0/4] register-field manipulation macros

[Jakub Kicinski]

Hi!

Looks like we're good to go :)

I've thrown in two mt7601u cleanups, macros are as approved
by Linus in the RFC.  I'm posting this already to give build
bot a head start while we wait for any additional feedback
on LKML.

-- v6 blurb:

This set moves to a global header file macros which I find
very useful and worth popularising.  The basic problem is
that since C bitfields are not very dependable accessing
subfields of registers becomes slightly inconvenient.
It is nice to have the necessary mask and shift operations
wrapped in a macro and have that macro compute shift
at compilation time using FFS.

My implementation follows what Felix Fietkau has done in
mt76.  Hannes Frederic Sowa suggested more use of standard
Linux/GCC functions.  Since the RFC I've also added a 
compile-time check to validate that the value passed to
setters fits in the mask.

I attempted the use of static inlines instead of macros
but it makes GCC < 6.0 barf at the BUILD_BUG_ON()s.
I also noticed that forcing arguments to be u32 for inlines
makes the compiler use 32bit arithmetic where it could
get away with 64bit before (on 64bit machines, obviously).
That's a potential performance concern but probably not
a very practical one today.  Apart from looking "cleaner"
static inlines would have the advantage that we could #undef
the auxiliary macros at the end of the header.

Build bot caught a build failure with -Os set.  AFAICT gcc
did not handle temporary variable I put in the macro
expression too well.  I work around that by defining
__BUILD_BUG_ON_NOT_POWER_OF_2 and using it instead of
BUILD_BUG_ON(!tmp || is_power_of_2(tmp)).

I'm planning to use those macros in another driver soon,
Felix may also use them when upstreaming mt76 if he chooses
to do so.

IMHO if accepted it would be best to push these through
Kalle's wireless drivers' tree, since the only existing
user is in drivers/net/wireless/.

v7:
 - drop the explicit type marking (u32/u64) - depend on the type
   of the mask instead;
 - only allow compilation time constant masks;
 - barf at "type of register too small to ever match mask" on get;
 - rename PUT -> PREP.
v6:
 - do a full rename in patch 2;
 - CC many people.
v5:
 - repost.
v4:
 - add documentation in the header.
v3:
 - don't use variables in statement expressions;
 - use __BUILD_BUG_ON_NOT_POWER_OF_2.
v2:
 - change Felix's email address.

Jakub Kicinski (4):
  add basic register-field manipulation macros
  mt7601u: remove redefinition of GENMASK
  mt7601u: remove unnecessary include
  mt7601u: use linux/bitfield.h

 drivers/net/wireless/mediatek/mt7601u/dma.c |  2 +-
 drivers/net/wireless/mediatek/mt7601u/dma.h | 10 ++-
 drivers/net/wireless/mediatek/mt7601u/eeprom.c  | 12 ++--
 drivers/net/wireless/mediatek/mt7601u/init.c| 10 +--
 drivers/net/wireless/mediatek/mt7601u/mac.c | 38 +-
 drivers/net/wireless/mediatek/mt7601u/main.c|  1 -
 drivers/net/wireless/mediatek/mt7601u/mcu.c | 20 +++---
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  4 +-
 drivers/net/wireless/mediatek/mt7601u/phy.c | 44 ++--
 drivers/net/wireless/mediatek/mt7601u/regs.h|  4 --
 drivers/net/wireless/mediatek/mt7601u/tx.c  | 19 ++---
 drivers/net/wireless/mediatek/mt7601u/util.h| 77 
 include/linux/bitfield.h| 93 +
 include/linux/bug.h |  3 +
 14 files changed, 177 insertions(+), 160 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h
 create mode 100644 include/linux/bitfield.h

-- 
1.9.1

[RFC (v7)] add basic register-field manipulation macros

[Jakub Kicinski]

Hi!

This is what I came up with.  Changes:
 - drop the explicit type marking (u32/u64) - depend on the type
   of the mask instead;
 - only allow compilation time constant masks;
 - barf at "type of register too small to ever match mask" on get;
 - rename PUT -> PREP.

I did not put the new marcos in bitops.h (where GENMASK lives)
because of the dependency on bug.h.  If I did there would be
a circular dependency like this:

bitops.h -> linux/bug.h -> asm-generic/bug.h -> kernel.h -> bitops.h

Which makes things really ugly.  I tried to remove the kernel.h
include from bug.h by separating panic-related definitions into
a new header.  There were still cycles coming from the notifier.h
which I had to include for panic notifier...

Reviewed-by: Dinan Gunawardena <dinan.gunaward...@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
---
 include/linux/bitfield.h | 93 
 include/linux/bug.h  |  3 ++
 2 files changed, 96 insertions(+)
 create mode 100644 include/linux/bitfield.h

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
new file mode 100644
index ..32ca8863e66d
--- /dev/null
+++ b/include/linux/bitfield.h
@@ -0,0 +1,93 @@
+/*
+ * Copyright (C) 2014 Felix Fietkau <n...@nbd.name>
+ * Copyright (C) 2004 - 2009 Ivo van Doorn <ivdo...@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _LINUX_BITFIELD_H
+#define _LINUX_BITFIELD_H
+
+#include 
+
+/*
+ * Bitfield access macros
+ *
+ * FIELD_{GET,PREP} macros take as first parameter shifted mask
+ * from which they extract the base mask and shift amount.
+ * Mask must be a compilation time constant.
+ *
+ * Example:
+ *
+ *  #define REG_FIELD_A  GENMASK(6, 0)
+ *  #define REG_FIELD_B  BIT(7)
+ *  #define REG_FIELD_C  GENMASK(15, 8)
+ *  #define REG_FIELD_D  GENMASK(31, 16)
+ *
+ * Get:
+ *  a = FIELD_GET(REG_FIELD_A, reg);
+ *  b = FIELD_GET(REG_FIELD_B, reg);
+ *
+ * Set:
+ *  reg = FIELD_PREP(REG_FIELD_A, 1) |
+ *   FIELD_PREP(REG_FIELD_B, 0) |
+ *   FIELD_PREP(REG_FIELD_C, c) |
+ *   FIELD_PREP(REG_FIELD_D, 0x40);
+ *
+ * Modify:
+ *  reg &= ~REG_FIELD_C;
+ *  reg |= FIELD_PREP(REG_FIELD_C, c);
+ */
+
+#define _bf_shf(x) (__builtin_ffsll(x) - 1)
+
+#define _BF_FIELD_CHECK(_mask, _reg, _val, _pfx)   \
+   ({  \
+   BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),  \
+_pfx "mask is not constant");  \
+   BUILD_BUG_ON_MSG(!(_mask), _pfx "mask is zero");\
+   BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?   \
+~((_mask) >> _bf_shf(_mask)) & (_val) : 0, \
+_pfx "value too large for the field"); \
+   BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \
+_pfx "type of reg too small for mask"); \
+   __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
+ (1ULL << _bf_shf(_mask))); \
+   })
+
+/**
+ * FIELD_PREP() - prepare a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_val:  value to put in the field
+ *
+ * FIELD_PREP() masks and shifts up the value.  The result should
+ * be combined with other fields of the bitfield using logical OR.
+ */
+#define FIELD_PREP(_mask, _val)
\
+   ({  \
+   _BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
+   ((typeof(_mask))(_val) << _bf_shf(_mask)) & (_mask);\
+   })
+
+/**
+ * FIELD_GET() - extract a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_reg:  32bit value of entire bitfield
+ *
+ * FIELD_GET() extracts the field specified by @_mask from the
+ * bitfield passed in as @_reg by masking and shifting it down.
+ */
+#define FIELD_GET(_mask, _reg) \
+   ({  \
+   _BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: ");\
+   (typeof(_mask))(((_reg) & (_mask)) >> _bf_shf(_mask));  \
+   })
+
+#endif
diff --git a/include/linux/bug.

Re: [PATCHv6 1/2] add basic register-field manipulation macros

[Jakub Kicinski]

On Wed, 17 Aug 2016 09:33:26 -0700, Linus Torvalds wrote:
> On Wed, Aug 17, 2016 at 3:31 AM, Kalle Valo  wrote:
> >
> > Are people ok with this? I think they are useful and I can take these
> > through my tree, but I would prefer to get an ack from other maintainers
> > first. Dave? Andrew?  
> 
> I'm not a huge fan, since the interface fundamentally seems to be
> oddly designed (why pass in the mask rather than "start bit +
> length"?).

Would that not require start and length to have separate defines?

I assume doing:

#define REG_BLA_FIELD_FOO  3, 4
val = FIELD_GET(REG_BLA_FIELD_FOO, reg);

is not acceptable.  Attempts to define a single value brought us to the
shifted mask.

> I also don't like how this very much would match the GENMASK() macros
> we have, but then it clashes with them in other ways
> 
>  - it's in a different header file

I'll move to bitops.h.

>  - it has completely different naming (GENMASK_ULL vs FIELD_GET64}.
> 
> I actually think the naming could/should be fixed by just
> automatically doing the right thing based on sizes.  For example,
> GENMASK could just have something like
> 
>   #define GENMASK(end,start) __builtin_choose_expr((end)>31,
> __GENMASK_64(end,start), __GENMASK_32(end,start))
> 
> and doing similar things with the FIELD_GET/SET ops.
> 
> So I'm not entirely happy about this all.

Seems feasible.

> But if people love the interface, and think the above kind of cleanups
> might be possible, I'd just want to make sure that there is also a
> 
>BUILD_BUG_ON(!__builtin_constant_p(_mask));
> 
> because if the mask isn't a build-time constant, the code ends up
> being *complete* shit. Also preferably something like
> 
>BUILD_BUG_ON((_mask) > (typeof(_val)~0ull);
> 
> to make sure you can't try to mask bits that don't exist in the value.
> 
> Or something like that to make mis-uses *really* obvious.

OK!

> The FIELD_PUT macro also seems misnamed. It doesn't "put" anything
> (unlike the GET macro). It just prepares the field for inserting
> later. As exemplified by how you actually have to put things:
> 
> First, "GET" - yeah, that looks like a "get" operation:
> 
>  * Get:
>  *  a = FIELD_GET(REG_FIELD_A, reg);
> 
> But then "PUT" isn't actually a PUT operation at all, but the comments
> kind of gloss over it by talking about "Modify" instead:
> 
>  * Modify:
>  *  reg &= ~REG_FIELD_C;
>  *  reg |= FIELD_PUT(REG_FIELD_C, c);
> 
> so I'm not entirely sure about the naming.
> 
> I can live with the FIELD_PUT naming, because I see how it comes
> about, even if I think it's a bit odd. I might have called it
> "FIELD_PREP" instead, but I'm not really sure that's all that much
> better.

Yes, it used to be called FIELD_SET, which was even worse.  I think
PREP sounds good.

Thanks!

[PATCHv6 2/2] mt7601u: use linux/bitfield.h

[Jakub Kicinski]

Use the newly added linux/bitfield.h.

Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
Reviewed-by: Dinan Gunawardena <dinan.gunaward...@netronome.com>
---
 drivers/net/wireless/mediatek/mt7601u/dma.c |  2 +-
 drivers/net/wireless/mediatek/mt7601u/dma.h | 10 ++--
 drivers/net/wireless/mediatek/mt7601u/eeprom.c  | 12 ++--
 drivers/net/wireless/mediatek/mt7601u/init.c|  9 +--
 drivers/net/wireless/mediatek/mt7601u/mac.c | 38 ++--
 drivers/net/wireless/mediatek/mt7601u/mcu.c | 18 +++---
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  4 +-
 drivers/net/wireless/mediatek/mt7601u/phy.c | 36 ++--
 drivers/net/wireless/mediatek/mt7601u/tx.c  | 16 ++---
 drivers/net/wireless/mediatek/mt7601u/util.h| 77 -
 10 files changed, 72 insertions(+), 150 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h

diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.c 
b/drivers/net/wireless/mediatek/mt7601u/dma.c
index 57a80cfa39b1..a8bc064bc14f 100644
--- a/drivers/net/wireless/mediatek/mt7601u/dma.c
+++ b/drivers/net/wireless/mediatek/mt7601u/dma.c
@@ -103,7 +103,7 @@ static void mt7601u_rx_process_seg(struct mt7601u_dev *dev, 
u8 *data,
 
if (unlikely(rxwi->zero[0] || rxwi->zero[1] || rxwi->zero[2]))
dev_err_once(dev->dev, "Error: RXWI zero fields are set\n");
-   if (unlikely(MT76_GET(MT_RXD_INFO_TYPE, fce_info)))
+   if (unlikely(FIELD_GET(MT_RXD_INFO_TYPE, fce_info)))
dev_err_once(dev->dev, "Error: RX path seen a non-pkt urb\n");
 
trace_mt_rx(dev, rxwi, fce_info);
diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.h 
b/drivers/net/wireless/mediatek/mt7601u/dma.h
index 978e8a90b87f..46c6a7860c38 100644
--- a/drivers/net/wireless/mediatek/mt7601u/dma.h
+++ b/drivers/net/wireless/mediatek/mt7601u/dma.h
@@ -18,8 +18,6 @@
 #include 
 #include 
 
-#include "util.h"
-
 #define MT_DMA_HDR_LEN 4
 #define MT_RX_INFO_LEN 4
 #define MT_FCE_INFO_LEN4
@@ -79,9 +77,9 @@ static inline int mt7601u_dma_skb_wrap(struct sk_buff *skb,
 */
 
info = flags |
-   MT76_SET(MT_TXD_INFO_LEN, round_up(skb->len, 4)) |
-   MT76_SET(MT_TXD_INFO_D_PORT, d_port) |
-   MT76_SET(MT_TXD_INFO_TYPE, type);
+   FIELD_PUT(MT_TXD_INFO_LEN, round_up(skb->len, 4)) |
+   FIELD_PUT(MT_TXD_INFO_D_PORT, d_port) |
+   FIELD_PUT(MT_TXD_INFO_TYPE, type);
 
put_unaligned_le32(info, skb_push(skb, sizeof(info)));
return skb_put_padto(skb, round_up(skb->len, 4) + 4);
@@ -90,7 +88,7 @@ static inline int mt7601u_dma_skb_wrap(struct sk_buff *skb,
 static inline int
 mt7601u_dma_skb_wrap_pkt(struct sk_buff *skb, enum mt76_qsel qsel, u32 flags)
 {
-   flags |= MT76_SET(MT_TXD_PKT_INFO_QSEL, qsel);
+   flags |= FIELD_PUT(MT_TXD_PKT_INFO_QSEL, qsel);
return mt7601u_dma_skb_wrap(skb, WLAN_PORT, DMA_PACKET, flags);
 }
 
diff --git a/drivers/net/wireless/mediatek/mt7601u/eeprom.c 
b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
index 8d8ee0344f7b..14ac2c0f8838 100644
--- a/drivers/net/wireless/mediatek/mt7601u/eeprom.c
+++ b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
@@ -45,8 +45,8 @@ mt7601u_efuse_read(struct mt7601u_dev *dev, u16 addr, u8 
*data,
val = mt76_rr(dev, MT_EFUSE_CTRL);
val &= ~(MT_EFUSE_CTRL_AIN |
 MT_EFUSE_CTRL_MODE);
-   val |= MT76_SET(MT_EFUSE_CTRL_AIN, addr & ~0xf) |
-  MT76_SET(MT_EFUSE_CTRL_MODE, mode) |
+   val |= FIELD_PUT(MT_EFUSE_CTRL_AIN, addr & ~0xf) |
+  FIELD_PUT(MT_EFUSE_CTRL_MODE, mode) |
   MT_EFUSE_CTRL_KICK;
mt76_wr(dev, MT_EFUSE_CTRL, val);
 
@@ -128,8 +128,8 @@ mt7601u_set_chip_cap(struct mt7601u_dev *dev, u8 *eeprom)
if (!field_valid(nic_conf0 >> 8))
return;
 
-   if (MT76_GET(MT_EE_NIC_CONF_0_RX_PATH, nic_conf0) > 1 ||
-   MT76_GET(MT_EE_NIC_CONF_0_TX_PATH, nic_conf0) > 1)
+   if (FIELD_GET(MT_EE_NIC_CONF_0_RX_PATH, nic_conf0) > 1 ||
+   FIELD_GET(MT_EE_NIC_CONF_0_TX_PATH, nic_conf0) > 1)
dev_err(dev->dev,
"Error: device has more than 1 RX/TX stream!\n");
 }
@@ -150,7 +150,7 @@ mt7601u_set_macaddr(struct mt7601u_dev *dev, const u8 
*eeprom)
 
mt76_wr(dev, MT_MAC_ADDR_DW0, get_unaligned_le32(dev->macaddr));
mt76_wr(dev, MT_MAC_ADDR_DW1, get_unaligned_le16(dev->macaddr + 4) |
-   MT76_SET(MT_MAC_ADDR_DW1_U2ME_MASK, 0xff));
+   FIELD_PUT(MT_MAC_ADDR_DW1_U2ME_MASK, 0xff));
 
return 0;
 }
@@ -176,7 +176,7 @@ mt7601u_set_channel_power(struct mt7601u_dev *dev, u8 
*eeprom)
u8 max_pwr;
 
val = mt7601u_rr(dev, MT_TX_ALC_CFG_0);
-   max_pwr = MT76_GET(MT_TX_ALC

[PATCHv6 1/2] add basic register-field manipulation macros

[Jakub Kicinski]

Common approach to accessing register fields is to define
structures or sets of macros containing mask and shift pair.
Operations on the register are then performed as follows:

 field = (reg >> shift) & mask;

 reg &= ~(mask << shift);
 reg |= (field & mask) << shift;

Defining shift and mask separately is tedious.  Ivo van Doorn
came up with an idea of computing them at compilation time
based on a single shifted mask (later refined by Felix) which
can be used like this:

 #define REG_FIELD 0x000ff000

 field = FIELD_GET(REG_FIELD, reg);

 reg &= ~REG_FIELD;
 reg |= FIELD_PUT(REG_FIELD, field);

FIELD_{GET,PUT} macros take care of finding out what the
appropriate shift is based on compilation time ffs operation.

GENMASK can be used to define registers (which is usually
less error-prone and easier to match with datasheets).

This approach is the most convenient I've seen so to limit code
multiplication let's move the macros to a global header file.
Attempts to use static inlines instead of macros failed due
to false positive triggering of BUILD_BUG_ON()s, especially with
GCC < 6.0.

Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
Reviewed-by: Dinan Gunawardena <dinan.gunaward...@netronome.com>
---
 include/linux/bitfield.h | 109 +++
 include/linux/bug.h  |   3 ++
 2 files changed, 112 insertions(+)
 create mode 100644 include/linux/bitfield.h

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
new file mode 100644
index ..ff9fd0af2ac7
--- /dev/null
+++ b/include/linux/bitfield.h
@@ -0,0 +1,109 @@
+/*
+ * Copyright (C) 2014 Felix Fietkau <n...@nbd.name>
+ * Copyright (C) 2004 - 2009 Ivo van Doorn <ivdo...@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _LINUX_BITFIELD_H
+#define _LINUX_BITFIELD_H
+
+#include 
+#include 
+
+#define _bf_shf(x) (__builtin_ffsll(x) - 1)
+
+#define _BF_FIELD_CHECK(_mask, _val)   \
+   ({  \
+   BUILD_BUG_ON(!(_mask)); \
+   BUILD_BUG_ON(__builtin_constant_p(_val) ?   \
+~((_mask) >> _bf_shf(_mask)) & (_val) :\
+0);\
+   __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
+ (1ULL << _bf_shf(_mask))); \
+   })
+
+/*
+ * Bitfield access macros
+ *
+ * This file contains macros which take as input shifted mask
+ * from which they extract the base mask and shift amount at
+ * compilation time.  There are two separate sets of the macros
+ * one for 32bit registers and one for 64bit ones.
+ *
+ * Fields can be defined using GENMASK (which is usually
+ * less error-prone and easier to match with datasheets).
+ *
+ * FIELD_{GET,PUT} macros are designed to be used with masks which
+ * are compilation time constants.
+ *
+ * Example:
+ *
+ *  #define REG_FIELD_A  GENMASK(6, 0)
+ *  #define REG_FIELD_B  BIT(7)
+ *  #define REG_FIELD_C  GENMASK(15, 8)
+ *  #define REG_FIELD_D  GENMASK(31, 16)
+ *
+ * Get:
+ *  a = FIELD_GET(REG_FIELD_A, reg);
+ *  b = FIELD_GET(REG_FIELD_B, reg);
+ *
+ * Set:
+ *  reg = FIELD_PUT(REG_FIELD_A, 1) |
+ *   FIELD_PUT(REG_FIELD_B, 0) |
+ *   FIELD_PUT(REG_FIELD_C, c) |
+ *   FIELD_PUT(REG_FIELD_D, 0x40);
+ *
+ * Modify:
+ *  reg &= ~REG_FIELD_C;
+ *  reg |= FIELD_PUT(REG_FIELD_C, c);
+ */
+
+/**
+ * FIELD_PUT() - construct a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_val:  value to put in the field
+ *
+ * FIELD_PUT() masks and shifts up the value.  The result should
+ * be combined with other fields of the bitfield using logical OR.
+ */
+#define FIELD_PUT(_mask, _val) \
+   ({  \
+   _BF_FIELD_CHECK(_mask, _val);   \
+   ((u32)(_val) << _bf_shf(_mask)) & (_mask);  \
+   })
+
+/**
+ * FIELD_GET() - extract a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_val:  32bit value of entire bitfield
+ *
+ * FIELD_GET() extracts the field specified by @_mask from the
+ * bitfield passed in as @_val.
+ */
+#define FIELD_GET(_mask, _val) \
+   ({ 

[PATCHv6 0/2] register-field manipulation macros

[Jakub Kicinski]

Hi!

This set moves to a global header file macros which I find
very useful and worth popularising.  The basic problem is
that since C bitfields are not very dependable accessing
subfields of registers becomes slightly inconvenient.
It is nice to have the necessary mask and shift operations
wrapped in a macro and have that macro compute shift
at compilation time using FFS.

My implementation follows what Felix Fietkau has done in
mt76.  Hannes Frederic Sowa suggested more use of standard
Linux/GCC functions.  Since the RFC I've also added a 
compile-time check to validate that the value passed to
setters fits in the mask.

I attempted the use of static inlines instead of macros
but it makes GCC < 6.0 barf at the BUILD_BUG_ON()s.
I also noticed that forcing arguments to be u32 for inlines
makes the compiler use 32bit arithmetic where it could
get away with 64bit before (on 64bit machines, obviously).
That's a potential performance concern but probably not
a very practical one today.  Apart from looking "cleaner"
static inlines would have the advantage that we could #undef
the auxiliary macros at the end of the header.

Build bot caught a build failure with -Os set.  AFAICT gcc
did not handle temporary variable I put in the macro
expression too well.  I work around that by defining
__BUILD_BUG_ON_NOT_POWER_OF_2 and using it instead of
BUILD_BUG_ON(!tmp || is_power_of_2(tmp)).

I'm planning to use those macros in another driver soon,
Felix may also use them when upstreaming mt76 if he chooses
to do so.

IMHO if accepted it would be best to push these through
Kalle's wireless drivers' tree, since the only existing
user is in drivers/net/wireless/.

As lowly device driver developers we would greatly
appreciate an Ack or a nod from the experts.

v6:
Do a full rename in patch 2.  CC many people.

v4:
 - add documentation in the header.
v3:
 - don't use variables in statement expressions;
 - use __BUILD_BUG_ON_NOT_POWER_OF_2.
v2:
 - change Felix's email address.

Jakub Kicinski (2):
  add basic register-field manipulation macros
  mt7601u: use linux/bitfield.h

 drivers/net/wireless/mediatek/mt7601u/dma.c |   2 +-
 drivers/net/wireless/mediatek/mt7601u/dma.h |  10 +--
 drivers/net/wireless/mediatek/mt7601u/eeprom.c  |  12 +--
 drivers/net/wireless/mediatek/mt7601u/init.c|   9 +-
 drivers/net/wireless/mediatek/mt7601u/mac.c |  38 -
 drivers/net/wireless/mediatek/mt7601u/mcu.c |  18 ++--
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |   4 +-
 drivers/net/wireless/mediatek/mt7601u/phy.c |  36 
 drivers/net/wireless/mediatek/mt7601u/tx.c  |  16 ++--
 drivers/net/wireless/mediatek/mt7601u/util.h|  77 -
 include/linux/bitfield.h| 109 
 include/linux/bug.h |   3 +
 12 files changed, 184 insertions(+), 150 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h
 create mode 100644 include/linux/bitfield.h

-- 
1.9.1

Re: [PATCHv5 wl-drv-next 0/2] register-field manipulation macros

[Jakub Kicinski]

On Wed,  6 Jul 2016 17:19:35 +0100, Jakub Kicinski wrote:
> Hi!
> 
> I've added few lines about the compilation problems in 
> the commit message of patch 1.  I would prefer the mass
> rename of macros in mt7601u not to be part of this series
> so patch 2 is left as it was and I'll follow up once this
> is accepted.

Hi Kalle,

what's the status of this set?  It's marked as 'Deferred' in
linux-wireless patchwork.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [oss-drivers] [PATCH][V2] nfp: check idx is -ENOSPC before using it is an index

[Jakub Kicinski]

On Mon, 11 Jul 2016 16:54:20 +0100, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> idx can be returned as -ENOSPC, so we should check for this first
> before using it as an index into nn->vxlan_usecnt[] to avoid an
> out of bounds array offset read.
> 
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>

Acked-by: Jakub Kicinski <jakub.kicin...@netronome.com>

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv5 wl-drv-next 0/2] register-field manipulation macros

[Jakub Kicinski]

Hi!

I've added few lines about the compilation problems in 
the commit message of patch 1.  I would prefer the mass
rename of macros in mt7601u not to be part of this series
so patch 2 is left as it was and I'll follow up once this
is accepted.

== v4 blurb

This set moves to a global header file macros which I find
very useful and worth popularising.  The basic problem is
that since C bitfields are not very dependable accessing
subfields of registers becomes slightly inconvenient.
It is nice to have the necessary mask and shift operations
wrapped in a macro.  It is also nice to have that macro
compute the shift amount based on the mask automatically.

My implementation follows what Felix Fietkau has done in
mt76.  Hannes Frederic Sowa suggested more use of standard
Linux/GCC functions.  Since the RFC I've also added a 
compile-time check to validate that the value passed to
setters fits in the mask.

I attempted the use of static inlines instead of macros
but it makes GCC < 6.0 barf at the BUILD_BUG_ON()s.
I also noticed that forcing arguments to be u32 for inlines
makes the compiler use 32bit arithmetic where it could
get away with 64bit before (on 64bit machines, obviously).
That's a potential performance concern but probably not
a very practical one today.  Apart from looking "cleaner"
static inlines would have the advantage that we could #undef
the auxiliary macros at the end of the header.

v3:
Build bot caught a build failure with -Os set.  AFAICT gcc
did not handle temporary variable I put in the macro
expression too well.  I work around that by defining
__BUILD_BUG_ON_NOT_POWER_OF_2 and using it instead of
BUILD_BUG_ON(!tmp || is_power_of_2(tmp)).

Please review and advise on improvements.

If accepted I think would be best to push this through
Kalle's tree, since the only existing user is in
drivers/net/wireless/.

v4:
 - add documentation in the header.
v3:
 - don't use variables in statement expressions;
 - use __BUILD_BUG_ON_NOT_POWER_OF_2.
v2:
 - change Felix's email address.

Jakub Kicinski (2):
  add basic register-field manipulation macros
  mt7601u: use linux/bitfield.h

 drivers/net/wireless/mediatek/mt7601u/dma.h |   2 -
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |   5 +-
 drivers/net/wireless/mediatek/mt7601u/util.h|  77 -
 include/linux/bitfield.h| 109 
 include/linux/bug.h |   3 +
 5 files changed, 116 insertions(+), 80 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h
 create mode 100644 include/linux/bitfield.h

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv5 wl-drv-next 2/2] mt7601u: use linux/bitfield.h

[Jakub Kicinski]

Use the newly added linux/bitfield.h.

Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
---
 drivers/net/wireless/mediatek/mt7601u/dma.h |  2 -
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  5 +-
 drivers/net/wireless/mediatek/mt7601u/util.h| 77 -
 3 files changed, 4 insertions(+), 80 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h

diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.h 
b/drivers/net/wireless/mediatek/mt7601u/dma.h
index 978e8a90b87f..166ac71905d2 100644
--- a/drivers/net/wireless/mediatek/mt7601u/dma.h
+++ b/drivers/net/wireless/mediatek/mt7601u/dma.h
@@ -18,8 +18,6 @@
 #include 
 #include 
 
-#include "util.h"
-
 #define MT_DMA_HDR_LEN 4
 #define MT_RX_INFO_LEN 4
 #define MT_FCE_INFO_LEN4
diff --git a/drivers/net/wireless/mediatek/mt7601u/mt7601u.h 
b/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
index 428bd2f10b7b..5ef62e02ce66 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
+++ b/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
@@ -15,6 +15,7 @@
 #ifndef MT7601U_H
 #define MT7601U_H
 
+#include 
 #include 
 #include 
 #include 
@@ -24,7 +25,6 @@
 #include 
 
 #include "regs.h"
-#include "util.h"
 
 #define MT_CALIBRATE_INTERVAL  (4 * HZ)
 
@@ -282,6 +282,9 @@ struct mt7601u_rxwi;
 
 extern const struct ieee80211_ops mt7601u_ops;
 
+#define MT76_SET   FIELD_PUT
+#define MT76_GET   FIELD_GET
+
 void mt7601u_init_debugfs(struct mt7601u_dev *dev);
 
 u32 mt7601u_rr(struct mt7601u_dev *dev, u32 offset);
diff --git a/drivers/net/wireless/mediatek/mt7601u/util.h 
b/drivers/net/wireless/mediatek/mt7601u/util.h
deleted file mode 100644
index b89140bf1210..
--- a/drivers/net/wireless/mediatek/mt7601u/util.h
+++ /dev/null
@@ -1,77 +0,0 @@
-/*
- * Copyright (C) 2014 Felix Fietkau <n...@openwrt.org>
- * Copyright (C) 2004 - 2009 Ivo van Doorn <ivdo...@gmail.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2
- * as published by the Free Software Foundation
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#ifndef __MT76_UTIL_H
-#define __MT76_UTIL_H
-
-/*
- * Power of two check, this will check
- * if the mask that has been given contains and contiguous set of bits.
- * Note that we cannot use the is_power_of_2() function since this
- * check must be done at compile-time.
- */
-#define is_power_of_two(x) ( !((x) & ((x)-1)) )
-#define low_bit_mask(x)( ((x)-1) & ~(x) )
-#define is_valid_mask(x)   is_power_of_two(1LU + (x) + low_bit_mask(x))
-
-/*
- * Macros to find first set bit in a variable.
- * These macros behave the same as the __ffs() functions but
- * the most important difference that this is done during
- * compile-time rather then run-time.
- */
-#define compile_ffs2(__x) \
-   __builtin_choose_expr(((__x) & 0x1), 0, 1)
-
-#define compile_ffs4(__x) \
-   __builtin_choose_expr(((__x) & 0x3), \
- (compile_ffs2((__x))), \
- (compile_ffs2((__x) >> 2) + 2))
-
-#define compile_ffs8(__x) \
-   __builtin_choose_expr(((__x) & 0xf), \
- (compile_ffs4((__x))), \
- (compile_ffs4((__x) >> 4) + 4))
-
-#define compile_ffs16(__x) \
-   __builtin_choose_expr(((__x) & 0xff), \
- (compile_ffs8((__x))), \
- (compile_ffs8((__x) >> 8) + 8))
-
-#define compile_ffs32(__x) \
-   __builtin_choose_expr(((__x) & 0x), \
- (compile_ffs16((__x))), \
- (compile_ffs16((__x) >> 16) + 16))
-
-/*
- * This macro will check the requirements for the FIELD{8,16,32} macros
- * The mask should be a constant non-zero contiguous set of bits which
- * does not exceed the given typelimit.
- */
-#define FIELD_CHECK(__mask) \
-   BUILD_BUG_ON(!(__mask) || !is_valid_mask(__mask))
-
-#define MT76_SET(_mask, _val)  \
-   ({  \
-   FIELD_CHECK(_mask); \
-   (((u32) (_val)) << compile_ffs32(_mask)) & _mask;   \
-   })
-
-#define MT76_GET(_mask, _val)  \
-   ({  \
-   FIELD_CHECK(_mask); \
-   (u32) (((_val) & _mask) >> compile_ffs32(_mas

[PATCHv5 wl-drv-next 1/2] add basic register-field manipulation macros

[Jakub Kicinski]

Common approach to accessing register fields is to define
structures or sets of macros containing mask and shift pair.
Operations on the register are then performed as follows:

 field = (reg >> shift) & mask;

 reg &= ~(mask << shift);
 reg |= (field & mask) << shift;

Defining shift and mask separately is tedious.  Ivo van Doorn
came up with an idea of computing them at compilation time
based on a single shifted mask (later refined by Felix) which
can be used like this:

 #define REG_FIELD 0x000ff000

 field = FIELD_GET(REG_FIELD, reg);

 reg &= ~REG_FIELD;
 reg |= FIELD_PUT(REG_FIELD, field);

FIELD_{GET,PUT} macros take care of finding out what the
appropriate shift is based on compilation time ffs operation.

GENMASK can be used to define registers (which is usually
less error-prone and easier to match with datasheets).

This approach is the most convenient I've seen so to limit code
multiplication let's move the macros to a global header file.
Attempts to use static inlines instead of macros failed due
to false positive triggering of BUILD_BUG_ON()s, especially with
GCC < 6.0.

Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
---
 include/linux/bitfield.h | 109 +++
 include/linux/bug.h  |   3 ++
 2 files changed, 112 insertions(+)
 create mode 100644 include/linux/bitfield.h

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
new file mode 100644
index ..ff9fd0af2ac7
--- /dev/null
+++ b/include/linux/bitfield.h
@@ -0,0 +1,109 @@
+/*
+ * Copyright (C) 2014 Felix Fietkau <n...@nbd.name>
+ * Copyright (C) 2004 - 2009 Ivo van Doorn <ivdo...@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _LINUX_BITFIELD_H
+#define _LINUX_BITFIELD_H
+
+#include 
+#include 
+
+#define _bf_shf(x) (__builtin_ffsll(x) - 1)
+
+#define _BF_FIELD_CHECK(_mask, _val)   \
+   ({  \
+   BUILD_BUG_ON(!(_mask)); \
+   BUILD_BUG_ON(__builtin_constant_p(_val) ?   \
+~((_mask) >> _bf_shf(_mask)) & (_val) :\
+0);\
+   __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
+ (1ULL << _bf_shf(_mask))); \
+   })
+
+/*
+ * Bitfield access macros
+ *
+ * This file contains macros which take as input shifted mask
+ * from which they extract the base mask and shift amount at
+ * compilation time.  There are two separate sets of the macros
+ * one for 32bit registers and one for 64bit ones.
+ *
+ * Fields can be defined using GENMASK (which is usually
+ * less error-prone and easier to match with datasheets).
+ *
+ * FIELD_{GET,PUT} macros are designed to be used with masks which
+ * are compilation time constants.
+ *
+ * Example:
+ *
+ *  #define REG_FIELD_A  GENMASK(6, 0)
+ *  #define REG_FIELD_B  BIT(7)
+ *  #define REG_FIELD_C  GENMASK(15, 8)
+ *  #define REG_FIELD_D  GENMASK(31, 16)
+ *
+ * Get:
+ *  a = FIELD_GET(REG_FIELD_A, reg);
+ *  b = FIELD_GET(REG_FIELD_B, reg);
+ *
+ * Set:
+ *  reg = FIELD_PUT(REG_FIELD_A, 1) |
+ *   FIELD_PUT(REG_FIELD_B, 0) |
+ *   FIELD_PUT(REG_FIELD_C, c) |
+ *   FIELD_PUT(REG_FIELD_D, 0x40);
+ *
+ * Modify:
+ *  reg &= ~REG_FIELD_C;
+ *  reg |= FIELD_PUT(REG_FIELD_C, c);
+ */
+
+/**
+ * FIELD_PUT() - construct a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_val:  value to put in the field
+ *
+ * FIELD_PUT() masks and shifts up the value.  The result should
+ * be combined with other fields of the bitfield using logical OR.
+ */
+#define FIELD_PUT(_mask, _val) \
+   ({  \
+   _BF_FIELD_CHECK(_mask, _val);   \
+   ((u32)(_val) << _bf_shf(_mask)) & (_mask);  \
+   })
+
+/**
+ * FIELD_GET() - extract a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_val:  32bit value of entire bitfield
+ *
+ * FIELD_GET() extracts the field specified by @_mask from the
+ * bitfield passed in as @_val.
+ */
+#define FIELD_GET(_mask, _val) \
+   ({  \
+   _BF_FIELD_CHECK(_mask,

[PATCHv4 wl-drv-next 2/2] mt7601u: use linux/bitfield.h

[Jakub Kicinski]

Use the newly added linux/bitfield.h.

Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
---
 drivers/net/wireless/mediatek/mt7601u/dma.h |  2 -
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  5 +-
 drivers/net/wireless/mediatek/mt7601u/util.h| 77 -
 3 files changed, 4 insertions(+), 80 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h

diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.h 
b/drivers/net/wireless/mediatek/mt7601u/dma.h
index 978e8a90b87f..166ac71905d2 100644
--- a/drivers/net/wireless/mediatek/mt7601u/dma.h
+++ b/drivers/net/wireless/mediatek/mt7601u/dma.h
@@ -18,8 +18,6 @@
 #include 
 #include 
 
-#include "util.h"
-
 #define MT_DMA_HDR_LEN 4
 #define MT_RX_INFO_LEN 4
 #define MT_FCE_INFO_LEN4
diff --git a/drivers/net/wireless/mediatek/mt7601u/mt7601u.h 
b/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
index 428bd2f10b7b..5ef62e02ce66 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
+++ b/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
@@ -15,6 +15,7 @@
 #ifndef MT7601U_H
 #define MT7601U_H
 
+#include 
 #include 
 #include 
 #include 
@@ -24,7 +25,6 @@
 #include 
 
 #include "regs.h"
-#include "util.h"
 
 #define MT_CALIBRATE_INTERVAL  (4 * HZ)
 
@@ -282,6 +282,9 @@ struct mt7601u_rxwi;
 
 extern const struct ieee80211_ops mt7601u_ops;
 
+#define MT76_SET   FIELD_PUT
+#define MT76_GET   FIELD_GET
+
 void mt7601u_init_debugfs(struct mt7601u_dev *dev);
 
 u32 mt7601u_rr(struct mt7601u_dev *dev, u32 offset);
diff --git a/drivers/net/wireless/mediatek/mt7601u/util.h 
b/drivers/net/wireless/mediatek/mt7601u/util.h
deleted file mode 100644
index b89140bf1210..
--- a/drivers/net/wireless/mediatek/mt7601u/util.h
+++ /dev/null
@@ -1,77 +0,0 @@
-/*
- * Copyright (C) 2014 Felix Fietkau <n...@openwrt.org>
- * Copyright (C) 2004 - 2009 Ivo van Doorn <ivdo...@gmail.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2
- * as published by the Free Software Foundation
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#ifndef __MT76_UTIL_H
-#define __MT76_UTIL_H
-
-/*
- * Power of two check, this will check
- * if the mask that has been given contains and contiguous set of bits.
- * Note that we cannot use the is_power_of_2() function since this
- * check must be done at compile-time.
- */
-#define is_power_of_two(x) ( !((x) & ((x)-1)) )
-#define low_bit_mask(x)( ((x)-1) & ~(x) )
-#define is_valid_mask(x)   is_power_of_two(1LU + (x) + low_bit_mask(x))
-
-/*
- * Macros to find first set bit in a variable.
- * These macros behave the same as the __ffs() functions but
- * the most important difference that this is done during
- * compile-time rather then run-time.
- */
-#define compile_ffs2(__x) \
-   __builtin_choose_expr(((__x) & 0x1), 0, 1)
-
-#define compile_ffs4(__x) \
-   __builtin_choose_expr(((__x) & 0x3), \
- (compile_ffs2((__x))), \
- (compile_ffs2((__x) >> 2) + 2))
-
-#define compile_ffs8(__x) \
-   __builtin_choose_expr(((__x) & 0xf), \
- (compile_ffs4((__x))), \
- (compile_ffs4((__x) >> 4) + 4))
-
-#define compile_ffs16(__x) \
-   __builtin_choose_expr(((__x) & 0xff), \
- (compile_ffs8((__x))), \
- (compile_ffs8((__x) >> 8) + 8))
-
-#define compile_ffs32(__x) \
-   __builtin_choose_expr(((__x) & 0x), \
- (compile_ffs16((__x))), \
- (compile_ffs16((__x) >> 16) + 16))
-
-/*
- * This macro will check the requirements for the FIELD{8,16,32} macros
- * The mask should be a constant non-zero contiguous set of bits which
- * does not exceed the given typelimit.
- */
-#define FIELD_CHECK(__mask) \
-   BUILD_BUG_ON(!(__mask) || !is_valid_mask(__mask))
-
-#define MT76_SET(_mask, _val)  \
-   ({  \
-   FIELD_CHECK(_mask); \
-   (((u32) (_val)) << compile_ffs32(_mask)) & _mask;   \
-   })
-
-#define MT76_GET(_mask, _val)  \
-   ({  \
-   FIELD_CHECK(_mask); \
-   (u32) (((_val) & _mask) >> compile_ffs32(_mas

[PATCHv4 wl-drv-next 1/2] add basic register-field manipulation macros

[Jakub Kicinski]

Common approach to accessing register fields is to define
structures or sets of macros containing mask and shift pair.
Operations on the register are then performed as follows:

 field = (reg >> shift) & mask;

 reg &= ~(mask << shift);
 reg |= (field & mask) << shift;

Defining shift and mask separately is tedious.  Ivo van Doorn
came up with an idea of computing them at compilation time
based on a single shifted mask (later refined by Felix) which
can be used like this:

 #define REG_FIELD 0x000ff000

 field = FIELD_GET(REG_FIELD, reg);

 reg &= ~REG_FIELD;
 reg |= FIELD_PUT(REG_FIELD, field);

FIELD_{GET,PUT} macros take care of finding out what the
appropriate shift is based on compilation time ffs operation.

GENMASK can be used to define registers (which is usually
less error-prone and easier to match with datasheets).

This approach is the most convenient I've seen so to limit code
multiplication let's move the macros to a global header file.

Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
---
 include/linux/bitfield.h | 109 +++
 include/linux/bug.h  |   3 ++
 2 files changed, 112 insertions(+)
 create mode 100644 include/linux/bitfield.h

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
new file mode 100644
index ..ff9fd0af2ac7
--- /dev/null
+++ b/include/linux/bitfield.h
@@ -0,0 +1,109 @@
+/*
+ * Copyright (C) 2014 Felix Fietkau <n...@nbd.name>
+ * Copyright (C) 2004 - 2009 Ivo van Doorn <ivdo...@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _LINUX_BITFIELD_H
+#define _LINUX_BITFIELD_H
+
+#include 
+#include 
+
+#define _bf_shf(x) (__builtin_ffsll(x) - 1)
+
+#define _BF_FIELD_CHECK(_mask, _val)   \
+   ({  \
+   BUILD_BUG_ON(!(_mask)); \
+   BUILD_BUG_ON(__builtin_constant_p(_val) ?   \
+~((_mask) >> _bf_shf(_mask)) & (_val) :\
+0);\
+   __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
+ (1ULL << _bf_shf(_mask))); \
+   })
+
+/*
+ * Bitfield access macros
+ *
+ * This file contains macros which take as input shifted mask
+ * from which they extract the base mask and shift amount at
+ * compilation time.  There are two separate sets of the macros
+ * one for 32bit registers and one for 64bit ones.
+ *
+ * Fields can be defined using GENMASK (which is usually
+ * less error-prone and easier to match with datasheets).
+ *
+ * FIELD_{GET,PUT} macros are designed to be used with masks which
+ * are compilation time constants.
+ *
+ * Example:
+ *
+ *  #define REG_FIELD_A  GENMASK(6, 0)
+ *  #define REG_FIELD_B  BIT(7)
+ *  #define REG_FIELD_C  GENMASK(15, 8)
+ *  #define REG_FIELD_D  GENMASK(31, 16)
+ *
+ * Get:
+ *  a = FIELD_GET(REG_FIELD_A, reg);
+ *  b = FIELD_GET(REG_FIELD_B, reg);
+ *
+ * Set:
+ *  reg = FIELD_PUT(REG_FIELD_A, 1) |
+ *   FIELD_PUT(REG_FIELD_B, 0) |
+ *   FIELD_PUT(REG_FIELD_C, c) |
+ *   FIELD_PUT(REG_FIELD_D, 0x40);
+ *
+ * Modify:
+ *  reg &= ~REG_FIELD_C;
+ *  reg |= FIELD_PUT(REG_FIELD_C, c);
+ */
+
+/**
+ * FIELD_PUT() - construct a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_val:  value to put in the field
+ *
+ * FIELD_PUT() masks and shifts up the value.  The result should
+ * be combined with other fields of the bitfield using logical OR.
+ */
+#define FIELD_PUT(_mask, _val) \
+   ({  \
+   _BF_FIELD_CHECK(_mask, _val);   \
+   ((u32)(_val) << _bf_shf(_mask)) & (_mask);  \
+   })
+
+/**
+ * FIELD_GET() - extract a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_val:  32bit value of entire bitfield
+ *
+ * FIELD_GET() extracts the field specified by @_mask from the
+ * bitfield passed in as @_val.
+ */
+#define FIELD_GET(_mask, _val) \
+   ({  \
+   _BF_FIELD_CHECK(_mask, 0);  \
+   (u32)(((_val)

[PATCHv4 wl-drv-next 0/2] register-field manipulation macros

[Jakub Kicinski]

Hi!

This set moves to a global header file macros which I find
very useful and worth popularising.  The basic problem is
that since C bitfields are not very dependable accessing
subfields of registers becomes slightly inconvenient.
It is nice to have the necessary mask and shift operations
wrapped in a macro.  It is also nice to have that macro
compute the shift amount based on the mask automatically.

My implementation follows what Felix Fietkau has done in
mt76.  Hannes Frederic Sowa suggested more use of standard
Linux/GCC functions.  Since the RFC I've also added a 
compile-time check to validate that the value passed to
setters fits in the mask.

I attempted the use of static inlines instead of macros
but it makes GCC < 6.0 barf at the BUILD_BUG_ON()s.
I also noticed that forcing arguments to be u32 for inlines
makes the compiler use 32bit arithmetic where it could
get away with 64bit before (on 64bit machines, obviously).
That's a potential performance concern but probably not
a very practical one today.  Apart from looking "cleaner"
static inlines would have the advantage that we could #undef
the auxiliary macros at the end of the header.

v3:
Build bot caught a build failure with -Os set.  AFAICT gcc
did not handle temporary variable I put in the macro
expression too well.  I work around that by defining
__BUILD_BUG_ON_NOT_POWER_OF_2 and using it instead of
BUILD_BUG_ON(!tmp || is_power_of_2(tmp)).

Please review and advise on improvements.

If accepted I think would be best to push this through
Kalle's tree, since the only existing user is in
drivers/net/wireless/.

v4:
 - add documentation in the header.
v3:
 - don't use variables in statement expressions;
 - use __BUILD_BUG_ON_NOT_POWER_OF_2.
v2:
 - change Felix's email address.

Jakub Kicinski (2):
  add basic register-field manipulation macros
  mt7601u: use linux/bitfield.h

 drivers/net/wireless/mediatek/mt7601u/dma.h |   2 -
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |   5 +-
 drivers/net/wireless/mediatek/mt7601u/util.h|  77 -
 include/linux/bitfield.h| 110 
 include/linux/bug.h |   3 +
 5 files changed, 117 insertions(+), 80 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h
 create mode 100644 include/linux/bitfield.h

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 wl-drv-next 1/2] add basic register-field manipulation macros

[Jakub Kicinski]

On Tue, 5 Jul 2016 10:56:13 +, David Laight wrote:
> From: Jakub Kicinski
> > Sent: 01 July 2016 22:27
> > 
> > C bitfields are problematic and best avoided.  Developers
> > interacting with hardware registers find themselves searching
> > for easy-to-use alternatives.  Common approach is to define
> > structures or sets of macros containing mask and shift pair.
> > Operations on the register are then performed as follows:
> > 
> >  field = (reg >> shift) & mask;
> > 
> >  reg &= ~(mask << shift);
> >  reg |= (field & mask) << shift;
> > 
> > Defining shift and mask separately is tedious.  Ivo van Doorn
> > came up with an idea of computing them at compilation time
> > based on a single shifted mask (later refined by Felix) which
> > can be used like this:
> > 
> >  #define REG_FIELD 0x000ff000
> > 
> >  field = FIELD_GET(REG_FIELD, reg);
> > 
> >  reg &= ~REG_FIELD;
> >  reg |= FIELD_PUT(REG_FIELD, field);  
> 
> My problem with these sort of 'helpers' is that they make it much harder
> to read code unless you happen to know exactly what the helpers do.
> Unexpected issues (like values being sign extended) can be hard to spot.
> 
> A lot of the time you can make things simpler by not doing the shifts
> (ie define shifted constants).

I think creating a standard set of macros in a global header file is
actually helping the problems you raise.  One is much more likely to
know exactly what a common macro is doing than some driver-specific
ad hoc macro.  Common macros are also much more likely to be well
tested making "unexpected issues" less likely to appear.

Defining constants is fine in 20% of cases when you have only a small
set of meaningful values (e.g. what to do for a 8 bit delay or
priority field?)  Besides when fields are not aligned to 4 bits it's
hard to tell from the shifted value what the base was.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv3 wl-drv-next 1/2] add basic register-field manipulation macros

[Jakub Kicinski]

C bitfields are problematic and best avoided.  Developers
interacting with hardware registers find themselves searching
for easy-to-use alternatives.  Common approach is to define
structures or sets of macros containing mask and shift pair.
Operations on the register are then performed as follows:

 field = (reg >> shift) & mask;

 reg &= ~(mask << shift);
 reg |= (field & mask) << shift;

Defining shift and mask separately is tedious.  Ivo van Doorn
came up with an idea of computing them at compilation time
based on a single shifted mask (later refined by Felix) which
can be used like this:

 #define REG_FIELD 0x000ff000

 field = FIELD_GET(REG_FIELD, reg);

 reg &= ~REG_FIELD;
 reg |= FIELD_PUT(REG_FIELD, field);

FIELD_{GET,PUT} macros take care of finding out what the
appropriate shift is based on compilation time ffs operation.

GENMASK can be used to define registers (which is usually
less error-prone and easier to match with datasheets).

This approach is the most convenient I've seen so to limit code
multiplication let's move the macros to a global header file.

Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
---
 include/linux/bitfield.h | 58 
 include/linux/bug.h  |  3 +++
 2 files changed, 61 insertions(+)
 create mode 100644 include/linux/bitfield.h

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
new file mode 100644
index ..d6a36c3c1775
--- /dev/null
+++ b/include/linux/bitfield.h
@@ -0,0 +1,58 @@
+/*
+ * Copyright (C) 2014 Felix Fietkau <n...@nbd.name>
+ * Copyright (C) 2004 - 2009 Ivo van Doorn <ivdo...@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _LINUX_BITFIELD_H
+#define _LINUX_BITFIELD_H
+
+#include 
+#include 
+#include 
+
+#define _bf_shf(x) (__builtin_ffsll(x) - 1)
+
+#define _BF_FIELD_CHECK(_mask, _val)   \
+   ({  \
+   BUILD_BUG_ON(!(_mask)); \
+   BUILD_BUG_ON(__builtin_constant_p(_val) ?   \
+~((_mask) >> _bf_shf(_mask)) & (_val) :\
+0);\
+   __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
+ (1ULL << _bf_shf(_mask))); \
+   })
+
+#define FIELD_PUT(_mask, _val) \
+   ({  \
+   _BF_FIELD_CHECK(_mask, _val);   \
+   ((u32)(_val) << _bf_shf(_mask)) & (_mask);  \
+   })
+
+#define FIELD_GET(_mask, _val) \
+   ({  \
+   _BF_FIELD_CHECK(_mask, 0);  \
+   (u32)(((_val) & (_mask)) >> _bf_shf(_mask));\
+   })
+
+#define FIELD_PUT64(_mask, _val)   \
+   ({  \
+   _BF_FIELD_CHECK(_mask, _val);   \
+   ((u64)(_val) << _bf_shf(_mask)) & (_mask);  \
+   })
+
+#define FIELD_GET64(_mask, _val)   \
+   ({  \
+   _BF_FIELD_CHECK(_mask, 0);  \
+   (u64)(((_val) & (_mask)) >> _bf_shf(_mask));\
+   })
+
+#endif
diff --git a/include/linux/bug.h b/include/linux/bug.h
index e51b0709e78d..bba5bdae1681 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -13,6 +13,7 @@ enum bug_trap_type {
 struct pt_regs;
 
 #ifdef __CHECKER__
+#define __BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
 #define BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
 #define BUILD_BUG_ON_ZERO(e) (0)
 #define BUILD_BUG_ON_NULL(e) ((void*)0)
@@ -24,6 +25,8 @@ struct pt_regs;
 #else /* __CHECKER__ */
 
 /* Force a compilation error if a constant expression is not a power of 2 */
+#define __BUILD_BUG_ON_NOT_POWER_OF_2(n)   \
+   BUILD_BUG_ON(((n) & ((n) - 1)) != 0)
 #define BUILD_BUG_ON_NOT_POWER_OF_2(n) \
BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv3 wl-drv-next 2/2] mt7601u: use linux/bitfield.h

[Jakub Kicinski]

Use the newly added linux/bitfield.h.

Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
---
 drivers/net/wireless/mediatek/mt7601u/dma.h |  2 -
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  5 +-
 drivers/net/wireless/mediatek/mt7601u/util.h| 77 -
 3 files changed, 4 insertions(+), 80 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h

diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.h 
b/drivers/net/wireless/mediatek/mt7601u/dma.h
index 978e8a90b87f..166ac71905d2 100644
--- a/drivers/net/wireless/mediatek/mt7601u/dma.h
+++ b/drivers/net/wireless/mediatek/mt7601u/dma.h
@@ -18,8 +18,6 @@
 #include 
 #include 
 
-#include "util.h"
-
 #define MT_DMA_HDR_LEN 4
 #define MT_RX_INFO_LEN 4
 #define MT_FCE_INFO_LEN4
diff --git a/drivers/net/wireless/mediatek/mt7601u/mt7601u.h 
b/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
index 428bd2f10b7b..5ef62e02ce66 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
+++ b/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
@@ -15,6 +15,7 @@
 #ifndef MT7601U_H
 #define MT7601U_H
 
+#include 
 #include 
 #include 
 #include 
@@ -24,7 +25,6 @@
 #include 
 
 #include "regs.h"
-#include "util.h"
 
 #define MT_CALIBRATE_INTERVAL  (4 * HZ)
 
@@ -282,6 +282,9 @@ struct mt7601u_rxwi;
 
 extern const struct ieee80211_ops mt7601u_ops;
 
+#define MT76_SET   FIELD_PUT
+#define MT76_GET   FIELD_GET
+
 void mt7601u_init_debugfs(struct mt7601u_dev *dev);
 
 u32 mt7601u_rr(struct mt7601u_dev *dev, u32 offset);
diff --git a/drivers/net/wireless/mediatek/mt7601u/util.h 
b/drivers/net/wireless/mediatek/mt7601u/util.h
deleted file mode 100644
index b89140bf1210..
--- a/drivers/net/wireless/mediatek/mt7601u/util.h
+++ /dev/null
@@ -1,77 +0,0 @@
-/*
- * Copyright (C) 2014 Felix Fietkau <n...@openwrt.org>
- * Copyright (C) 2004 - 2009 Ivo van Doorn <ivdo...@gmail.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2
- * as published by the Free Software Foundation
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#ifndef __MT76_UTIL_H
-#define __MT76_UTIL_H
-
-/*
- * Power of two check, this will check
- * if the mask that has been given contains and contiguous set of bits.
- * Note that we cannot use the is_power_of_2() function since this
- * check must be done at compile-time.
- */
-#define is_power_of_two(x) ( !((x) & ((x)-1)) )
-#define low_bit_mask(x)( ((x)-1) & ~(x) )
-#define is_valid_mask(x)   is_power_of_two(1LU + (x) + low_bit_mask(x))
-
-/*
- * Macros to find first set bit in a variable.
- * These macros behave the same as the __ffs() functions but
- * the most important difference that this is done during
- * compile-time rather then run-time.
- */
-#define compile_ffs2(__x) \
-   __builtin_choose_expr(((__x) & 0x1), 0, 1)
-
-#define compile_ffs4(__x) \
-   __builtin_choose_expr(((__x) & 0x3), \
- (compile_ffs2((__x))), \
- (compile_ffs2((__x) >> 2) + 2))
-
-#define compile_ffs8(__x) \
-   __builtin_choose_expr(((__x) & 0xf), \
- (compile_ffs4((__x))), \
- (compile_ffs4((__x) >> 4) + 4))
-
-#define compile_ffs16(__x) \
-   __builtin_choose_expr(((__x) & 0xff), \
- (compile_ffs8((__x))), \
- (compile_ffs8((__x) >> 8) + 8))
-
-#define compile_ffs32(__x) \
-   __builtin_choose_expr(((__x) & 0x), \
- (compile_ffs16((__x))), \
- (compile_ffs16((__x) >> 16) + 16))
-
-/*
- * This macro will check the requirements for the FIELD{8,16,32} macros
- * The mask should be a constant non-zero contiguous set of bits which
- * does not exceed the given typelimit.
- */
-#define FIELD_CHECK(__mask) \
-   BUILD_BUG_ON(!(__mask) || !is_valid_mask(__mask))
-
-#define MT76_SET(_mask, _val)  \
-   ({  \
-   FIELD_CHECK(_mask); \
-   (((u32) (_val)) << compile_ffs32(_mask)) & _mask;   \
-   })
-
-#define MT76_GET(_mask, _val)  \
-   ({  \
-   FIELD_CHECK(_mask); \
-   (u32) (((_val) & _mask) >> compile_ffs32(_mas

[PATCHv3 wl-drv-next 0/2] register-field manipulation macros

[Jakub Kicinski]

Hi!

This set moves to a global header file macros which I find
very useful and worth popularising.  The basic problem is
that since C bitfields are not very dependable accessing
subfields of registers becomes slightly inconvenient.
It is nice to have the necessary mask and shift operations
wrapped in a macro.  It is also nice to have that macro
compute the shift amount based on the mask automatically.

My implementation follows what Felix Fietkau has done in
mt76.  Hannes Frederic Sowa suggested more use of standard
Linux/GCC functions.  Since the RFC I've also added a 
compile-time check to validate that the value passed to
setters fits in the mask.

I attempted the use of static inlines instead of macros
but it makes GCC < 6.0 barf at the BUILD_BUG_ON()s.
I also noticed that forcing arguments to be u32 for inlines
makes the compiler use 32bit arithmetic where it could
get away with 64bit before (on 64bit machines, obviously).
That's a potential performance concern but probably not
a very practical one today.  Apart from looking "cleaner"
static inlines would have the advantage that we could #undef
the auxiliary macros at the end of the header.

v3:
Build bot caught a build failure with -Os set.  AFAICT gcc
did not handle temporary variable I put in the macro
expression too well.  I work around that by defining
__BUILD_BUG_ON_NOT_POWER_OF_2 and using it instead of
BUILD_BUG_ON(!tmp || is_power_of_2(tmp)).

Please review and advise on improvements.

If accepted I think would be best to push this through
Kalle's tree, since the only existing user is in
drivers/net/wireless/.

v3:
 - don't use variables in statement expressions;
 - use __BUILD_BUG_ON_NOT_POWER_OF_2.
v2:
 - change Felix's email address.

Jakub Kicinski (2):
  add basic register-field manipulation macros
  mt7601u: use linux/bitfield.h

 drivers/net/wireless/mediatek/mt7601u/dma.h |  2 -
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  5 +-
 drivers/net/wireless/mediatek/mt7601u/util.h| 77 -
 include/linux/bitfield.h| 58 +++
 include/linux/bug.h |  3 +
 5 files changed, 65 insertions(+), 80 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h
 create mode 100644 include/linux/bitfield.h

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 1/2] add basic register-field manipulation macros

[Jakub Kicinski]

On Tue, 14 Jun 2016 20:53:28 +0200, Arend van Spriel wrote:
> On 14-06-16 13:44, Jakub Kicinski wrote:
> > +#ifndef _LINUX_BITFIELD_H
> > +#define _LINUX_BITFIELD_H
> > +
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define _bf_shf(x) (__builtin_ffsll(x) - 1)
> > +
> > +#define _BF_FIELD_CHECK(_mask, _val)   
> > \
> > +   ({  \
> > +   const u64 hi = (_mask) + (1ULL << _bf_shf(_mask));  \
> > +   \
> > +   BUILD_BUG_ON(!(_mask) || (hi && !is_power_of_2_u64(hi))); \
> > +   BUILD_BUG_ON(__builtin_constant_p(_val) ?   \
> > +~((_mask) >> _bf_shf(_mask)) & (_val) :\
> > +0);\
> > +   })  
> 
> I am sceptic whether it is useful to have 64-bit used here and there is
> a price to pay on (many) 32-bit architectures for using 64-bit
> operations. Maybe it is not an issue because it is inside BUILD_BUG_ON()
> macro.

It's a trade-off between having a separate set of macros for 32-bit
machines and risking someone potentially loosing cycles when using the
macros in a non-standard way.  Note that all 64 bit operations are
performed on the mask which I expect to be compile time constant.

> > +#define FIELD_PUT(_mask, _val) \
> > +   ({  \
> > +   _BF_FIELD_CHECK(_mask, _val);   \
> > +   ((u32)(_val) << _bf_shf(_mask)) & (_mask);  \
> > +   })
> > +
> > +#define FIELD_GET(_mask, _val) \
> > +   ({  \
> > +   _BF_FIELD_CHECK(_mask, 0);  \
> > +   (u32)(((_val) & (_mask)) >> _bf_shf(_mask));\
> > +   })
> > +
> > +#define FIELD_PUT64(_mask, _val)   \
> > +   ({  \
> > +   _BF_FIELD_CHECK(_mask, _val);   \
> > +   ((u64)(_val) << _bf_shf(_mask)) & (_mask);  \
> > +   })
> > +
> > +#define FIELD_GET64(_mask, _val)   \
> > +   ({  \
> > +   _BF_FIELD_CHECK(_mask, 0);  \
> > +   (u64)(((_val) & (_mask)) >> _bf_shf(_mask));\
> > +   })  
> 
> Is there really hardware out there that exposes 64-bit wide hardware
> registers?

I need this for encoding 64-bit wide RISC instructions [1] to be honest.

[1] http://thread.gmane.org/gmane.linux.network/414538
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 0/2] register-field manipulation macros

[Jakub Kicinski]

Hi!

This set moves to a global header file macros which I find
very useful and worth popularising.  The basic problem is
that since C bitfields are not very dependable accessing
subfields of registers becomes slightly inconvenient.
It is nice to have the necessary mask and shift operations
wrapped in a macro.  It is also nice to have that macro
compute the shift amount based on the mask automatically.

My implementation follows what Felix Fietkau has done in
mt76.  Hannes Frederic Sowa suggested more use of standard
Linux/GCC functions.  Since the RFC I've also added a 
compile-time check to validate that the value passed to
setters fits in the mask.

I attempted the use of static inlines instead of macros
but it makes GCC < 6.0 barf at the BUILD_BUG_ON()s.
I also noticed that forcing arguments to be u32 for inlines
makes the compiler use 32bit arithmetic where it could
get away with 64bit before (on 64bit machines, obviously).
That's a potential performance concern but probably not
a very practical one today.  Apart from looking "cleaner"
static inlines would have the advantage that we could #undef
the auxiliary macros at the end of the header.

Please review and advise on improvements.

If accepted I think would be best to push this through
Kalle's tree, since the only existing user is in
drivers/net/wireless/.

v2:
 - change Felix's email address.

Jakub Kicinski (2):
  add basic register-field manipulation macros
  mt7601u: use linux/bitfield.h

 drivers/net/wireless/mediatek/mt7601u/dma.h |  2 -
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  5 +-
 drivers/net/wireless/mediatek/mt7601u/util.h| 77 -
 include/linux/bitfield.h| 58 +++
 include/linux/log2.h|  6 ++
 5 files changed, 68 insertions(+), 80 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h
 create mode 100644 include/linux/bitfield.h

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 1/2] add basic register-field manipulation macros

[Jakub Kicinski]

C bitfields are problematic and best avoided.  Developers
interacting with hardware registers find themselves searching
for easy-to-use alternatives.  Common approach is to define
structures or sets of macros containing mask and shift pair.
Operations on the register are then performed as follows:

 field = (reg >> shift) & mask;

 reg &= ~(mask << shift);
 reg |= (field & mask) << shift;

Defining shift and mask separately is tedious.  Ivo van Doorn
came up with an idea of computing them at compilation time
based on a single shifted mask (later refined by Felix) which
can be used like this:

 #define X_REG_FIELD 0x000ff000

 field = FIELD_GET(X_REG_FIELD, reg);

 reg &= ~X_REG_FIELD;
 reg |= FIELD_PUT(X_REG_FIELD, field);

FIELD_{GET,PUT} macros take care of finding out what the
appropriate shift is based on compilation time ffs operation.

GENMASK can be used to define registers (which is usually
less error-prone and easier to match with datasheets).

This approach is the most convenient I've seen so to limit code
multiplication let's move the macros to a global header file.

Compared to Felix Fietkau's implementation from mt76 this one
uses standard Linux and GCC functions such as is_power_of_2()
and __builtin_ffsll().

Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
---
v2:
 - change Felix's email address.

 include/linux/bitfield.h | 58 
 include/linux/log2.h |  6 +
 2 files changed, 64 insertions(+)
 create mode 100644 include/linux/bitfield.h

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
new file mode 100644
index ..9560d1877cbc
--- /dev/null
+++ b/include/linux/bitfield.h
@@ -0,0 +1,58 @@
+/*
+ * Copyright (C) 2014 Felix Fietkau <n...@nbd.name>
+ * Copyright (C) 2004 - 2009 Ivo van Doorn <ivdo...@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _LINUX_BITFIELD_H
+#define _LINUX_BITFIELD_H
+
+#include 
+#include 
+#include 
+
+#define _bf_shf(x) (__builtin_ffsll(x) - 1)
+
+#define _BF_FIELD_CHECK(_mask, _val)   \
+   ({  \
+   const u64 hi = (_mask) + (1ULL << _bf_shf(_mask));  \
+   \
+   BUILD_BUG_ON(!(_mask) || (hi && !is_power_of_2_u64(hi))); \
+   BUILD_BUG_ON(__builtin_constant_p(_val) ?   \
+~((_mask) >> _bf_shf(_mask)) & (_val) :\
+0);\
+   })
+
+#define FIELD_PUT(_mask, _val) \
+   ({  \
+   _BF_FIELD_CHECK(_mask, _val);   \
+   ((u32)(_val) << _bf_shf(_mask)) & (_mask);  \
+   })
+
+#define FIELD_GET(_mask, _val) \
+   ({  \
+   _BF_FIELD_CHECK(_mask, 0);  \
+   (u32)(((_val) & (_mask)) >> _bf_shf(_mask));\
+   })
+
+#define FIELD_PUT64(_mask, _val)   \
+   ({  \
+   _BF_FIELD_CHECK(_mask, _val);   \
+   ((u64)(_val) << _bf_shf(_mask)) & (_mask);  \
+   })
+
+#define FIELD_GET64(_mask, _val)   \
+   ({  \
+   _BF_FIELD_CHECK(_mask, 0);  \
+   (u64)(((_val) & (_mask)) >> _bf_shf(_mask));\
+   })
+
+#endif
diff --git a/include/linux/log2.h b/include/linux/log2.h
index fd7ff3d91e6a..1b5e1f4da043 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h
@@ -54,6 +54,12 @@ bool is_power_of_2(unsigned long n)
return (n != 0 && ((n & (n - 1)) == 0));
 }
 
+static inline __attribute__((const))
+bool is_power_of_2_u64(u64 n)
+{
+   return (n != 0 && ((n & (n - 1)) == 0));
+}
+
 /*
  * round up to nearest power of two
  */
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 2/2] mt7601u: use linux/bitfield.h

[Jakub Kicinski]

Use the newly added linux/bitfield.h.

Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
---
 drivers/net/wireless/mediatek/mt7601u/dma.h |  2 -
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  5 +-
 drivers/net/wireless/mediatek/mt7601u/util.h| 77 -
 3 files changed, 4 insertions(+), 80 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h

diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.h 
b/drivers/net/wireless/mediatek/mt7601u/dma.h
index 978e8a90b87f..166ac71905d2 100644
--- a/drivers/net/wireless/mediatek/mt7601u/dma.h
+++ b/drivers/net/wireless/mediatek/mt7601u/dma.h
@@ -18,8 +18,6 @@
 #include 
 #include 
 
-#include "util.h"
-
 #define MT_DMA_HDR_LEN 4
 #define MT_RX_INFO_LEN 4
 #define MT_FCE_INFO_LEN4
diff --git a/drivers/net/wireless/mediatek/mt7601u/mt7601u.h 
b/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
index 428bd2f10b7b..5ef62e02ce66 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
+++ b/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
@@ -15,6 +15,7 @@
 #ifndef MT7601U_H
 #define MT7601U_H
 
+#include 
 #include 
 #include 
 #include 
@@ -24,7 +25,6 @@
 #include 
 
 #include "regs.h"
-#include "util.h"
 
 #define MT_CALIBRATE_INTERVAL  (4 * HZ)
 
@@ -282,6 +282,9 @@ struct mt7601u_rxwi;
 
 extern const struct ieee80211_ops mt7601u_ops;
 
+#define MT76_SET   FIELD_PUT
+#define MT76_GET   FIELD_GET
+
 void mt7601u_init_debugfs(struct mt7601u_dev *dev);
 
 u32 mt7601u_rr(struct mt7601u_dev *dev, u32 offset);
diff --git a/drivers/net/wireless/mediatek/mt7601u/util.h 
b/drivers/net/wireless/mediatek/mt7601u/util.h
deleted file mode 100644
index b89140bf1210..
--- a/drivers/net/wireless/mediatek/mt7601u/util.h
+++ /dev/null
@@ -1,77 +0,0 @@
-/*
- * Copyright (C) 2014 Felix Fietkau <n...@openwrt.org>
- * Copyright (C) 2004 - 2009 Ivo van Doorn <ivdo...@gmail.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2
- * as published by the Free Software Foundation
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#ifndef __MT76_UTIL_H
-#define __MT76_UTIL_H
-
-/*
- * Power of two check, this will check
- * if the mask that has been given contains and contiguous set of bits.
- * Note that we cannot use the is_power_of_2() function since this
- * check must be done at compile-time.
- */
-#define is_power_of_two(x) ( !((x) & ((x)-1)) )
-#define low_bit_mask(x)( ((x)-1) & ~(x) )
-#define is_valid_mask(x)   is_power_of_two(1LU + (x) + low_bit_mask(x))
-
-/*
- * Macros to find first set bit in a variable.
- * These macros behave the same as the __ffs() functions but
- * the most important difference that this is done during
- * compile-time rather then run-time.
- */
-#define compile_ffs2(__x) \
-   __builtin_choose_expr(((__x) & 0x1), 0, 1)
-
-#define compile_ffs4(__x) \
-   __builtin_choose_expr(((__x) & 0x3), \
- (compile_ffs2((__x))), \
- (compile_ffs2((__x) >> 2) + 2))
-
-#define compile_ffs8(__x) \
-   __builtin_choose_expr(((__x) & 0xf), \
- (compile_ffs4((__x))), \
- (compile_ffs4((__x) >> 4) + 4))
-
-#define compile_ffs16(__x) \
-   __builtin_choose_expr(((__x) & 0xff), \
- (compile_ffs8((__x))), \
- (compile_ffs8((__x) >> 8) + 8))
-
-#define compile_ffs32(__x) \
-   __builtin_choose_expr(((__x) & 0x), \
- (compile_ffs16((__x))), \
- (compile_ffs16((__x) >> 16) + 16))
-
-/*
- * This macro will check the requirements for the FIELD{8,16,32} macros
- * The mask should be a constant non-zero contiguous set of bits which
- * does not exceed the given typelimit.
- */
-#define FIELD_CHECK(__mask) \
-   BUILD_BUG_ON(!(__mask) || !is_valid_mask(__mask))
-
-#define MT76_SET(_mask, _val)  \
-   ({  \
-   FIELD_CHECK(_mask); \
-   (((u32) (_val)) << compile_ffs32(_mask)) & _mask;   \
-   })
-
-#define MT76_GET(_mask, _val)  \
-   ({  \
-   FIELD_CHECK(_mask); \
-   (u32) (((_val) & _mask) >> compile_ffs32(_mas

Re: [PATCH v2 23/27] rt2x00: move under ralink vendor directory

[Jakub Kicinski]

On Wed, 18 Nov 2015 16:46:02 +0200, Kalle Valo wrote:
> Part of reorganising wireless drivers directory and Kconfig.
> 
> Signed-off-by: Kalle Valo 

For Ralink you could probably drop the rt2x00 directory.  RaLink Tech.
doesn't exist any more and rt2x00 contains drivers for all of their
devices.

Obviously this is just a suggestion, not a show stopper.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH -next 2/4] mt7601u: use correct ieee80211_rx variant

[Jakub Kicinski]

From: Jakub Kicinski kubak...@wp.pl

Rx is run inside a tasklet so ieee80211_rx() should be used
instead of ieee80211_rx_ni().

Signed-off-by: Jakub Kicinski kubak...@wp.pl
---
 drivers/net/wireless/mediatek/mt7601u/dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.c 
b/drivers/net/wireless/mediatek/mt7601u/dma.c
index 7217da4f1543..fb183e369d92 100644
--- a/drivers/net/wireless/mediatek/mt7601u/dma.c
+++ b/drivers/net/wireless/mediatek/mt7601u/dma.c
@@ -112,7 +112,7 @@ static void mt7601u_rx_process_seg(struct mt7601u_dev *dev, 
u8 *data,
if (!skb)
return;
 
-   ieee80211_rx_ni(dev-hw, skb);
+   ieee80211_rx(dev-hw, skb);
 }
 
 static u16 mt7601u_rx_next_seg_len(u8 *data, u32 data_len)
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH -next 3/4] mt7601u: fix tx status reporting contexts

[Jakub Kicinski]

From: Jakub Kicinski kubak...@wp.pl

mac80211 requires that rx path does not run concurrently with
tx status reporting.  Since rx path is run in driver tasklet,
tx status cannot be reported directly from interrupt context
(there would be no way to lock it out).

Add tasklet for tx and move all possible code from irq handler
there.

Note: tx tasklet is needed because workqueue is queued very
  rarely and that kills TCP performance.

Signed-off-by: Jakub Kicinski kubak...@wp.pl
---
 drivers/net/wireless/mediatek/mt7601u/dma.c | 30 +
 drivers/net/wireless/mediatek/mt7601u/init.c|  1 +
 drivers/net/wireless/mediatek/mt7601u/mac.c |  4 
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  2 ++
 4 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.c 
b/drivers/net/wireless/mediatek/mt7601u/dma.c
index fb183e369d92..63c485443a38 100644
--- a/drivers/net/wireless/mediatek/mt7601u/dma.c
+++ b/drivers/net/wireless/mediatek/mt7601u/dma.c
@@ -236,23 +236,42 @@ static void mt7601u_complete_tx(struct urb *urb)
skb = q-e[q-start].skb;
trace_mt_tx_dma_done(dev, skb);
 
-   mt7601u_tx_status(dev, skb);
+   __skb_queue_tail(dev-tx_skb_done, skb);
+   tasklet_schedule(dev-tx_tasklet);
 
if (q-used == q-entries - q-entries / 8)
ieee80211_wake_queue(dev-hw, skb_get_queue_mapping(skb));
 
q-start = (q-start + 1) % q-entries;
q-used--;
+out:
+   spin_unlock_irqrestore(dev-tx_lock, flags);
+}
 
-   if (urb-status)
-   goto out;
+static void mt7601u_tx_tasklet(unsigned long data)
+{
+   struct mt7601u_dev *dev = (struct mt7601u_dev *) data;
+   struct sk_buff_head skbs;
+   unsigned long flags;
+
+   __skb_queue_head_init(skbs);
+
+   spin_lock_irqsave(dev-tx_lock, flags);
 
set_bit(MT7601U_STATE_MORE_STATS, dev-state);
if (!test_and_set_bit(MT7601U_STATE_READING_STATS, dev-state))
queue_delayed_work(dev-stat_wq, dev-stat_work,
   msecs_to_jiffies(10));
-out:
+
+   skb_queue_splice_init(dev-tx_skb_done, skbs);
+
spin_unlock_irqrestore(dev-tx_lock, flags);
+
+   while (!skb_queue_empty(skbs)) {
+   struct sk_buff *skb = __skb_dequeue(skbs);
+
+   mt7601u_tx_status(dev, skb);
+   }
 }
 
 static int mt7601u_dma_submit_tx(struct mt7601u_dev *dev,
@@ -475,6 +494,7 @@ int mt7601u_dma_init(struct mt7601u_dev *dev)
 {
int ret = -ENOMEM;
 
+   tasklet_init(dev-tx_tasklet, mt7601u_tx_tasklet, (unsigned long) dev);
tasklet_init(dev-rx_tasklet, mt7601u_rx_tasklet, (unsigned long) dev);
 
ret = mt7601u_alloc_tx(dev);
@@ -502,4 +522,6 @@ void mt7601u_dma_cleanup(struct mt7601u_dev *dev)
 
mt7601u_free_rx(dev);
mt7601u_free_tx(dev);
+
+   tasklet_kill(dev-tx_tasklet);
 }
diff --git a/drivers/net/wireless/mediatek/mt7601u/init.c 
b/drivers/net/wireless/mediatek/mt7601u/init.c
index df3dd56199a7..38eb20ba6e58 100644
--- a/drivers/net/wireless/mediatek/mt7601u/init.c
+++ b/drivers/net/wireless/mediatek/mt7601u/init.c
@@ -456,6 +456,7 @@ struct mt7601u_dev *mt7601u_alloc_device(struct device 
*pdev)
spin_lock_init(dev-lock);
spin_lock_init(dev-con_mon_lock);
atomic_set(dev-avg_ampdu_len, 1);
+   skb_queue_head_init(dev-tx_skb_done);
 
dev-stat_wq = alloc_workqueue(mt7601u, WQ_UNBOUND, 0);
if (!dev-stat_wq) {
diff --git a/drivers/net/wireless/mediatek/mt7601u/mac.c 
b/drivers/net/wireless/mediatek/mt7601u/mac.c
index 7514bce1ac91..e3928cfa3d63 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mac.c
+++ b/drivers/net/wireless/mediatek/mt7601u/mac.c
@@ -181,7 +181,11 @@ void mt76_send_tx_status(struct mt7601u_dev *dev, struct 
mt76_tx_status *stat)
}
 
mt76_mac_fill_tx_status(dev, info, stat);
+
+   local_bh_disable();
ieee80211_tx_status_noskb(dev-hw, sta, info);
+   local_bh_enable();
+
rcu_read_unlock();
 }
 
diff --git a/drivers/net/wireless/mediatek/mt7601u/mt7601u.h 
b/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
index 6bdfc1103fcc..bc5e294feb8c 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
+++ b/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
@@ -199,7 +199,9 @@ struct mt7601u_dev {
 
/* TX */
spinlock_t tx_lock;
+   struct tasklet_struct tx_tasklet;
struct mt7601u_tx_queue *tx_q;
+   struct sk_buff_head tx_skb_done;
 
atomic_t avg_ampdu_len;
 
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH -next 1/4] mt7601u: fix dma from stack address

[Jakub Kicinski]

From: Jakub Kicinski kubak...@wp.pl

DMA to variables located on the stack is a bad idea.
For simplicity and to avoid frequent allocations create
a buffer inside the device structure.  Protect this
buffer with vendor_req_mutex.  Don't protect vendor
requests which don't use this buffer.

Signed-off-by: Jakub Kicinski kubak...@wp.pl
---
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  4 +-
 drivers/net/wireless/mediatek/mt7601u/usb.c | 63 -
 drivers/net/wireless/mediatek/mt7601u/usb.h |  2 +
 3 files changed, 34 insertions(+), 35 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt7601u/mt7601u.h 
b/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
index 9102be6b95cb..6bdfc1103fcc 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
+++ b/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
@@ -146,7 +146,7 @@ enum {
  * @rx_lock:   protects @rx_q.
  * @con_mon_lock:  protects @ap_bssid, @bcn_*, @avg_rssi.
  * @mutex: ensures exclusive access from mac80211 callbacks.
- * @vendor_req_mutex:  ensures atomicity of vendor requests.
+ * @vendor_req_mutex:  protects @vend_buf, ensures atomicity of split writes.
  * @reg_atomic_mutex:  ensures atomicity of indirect register accesses
  * (accesses to RF and BBP).
  * @hw_atomic_mutex:   ensures exclusive access to HW during critical
@@ -184,6 +184,8 @@ struct mt7601u_dev {
struct mt7601u_eeprom_params *ee;
 
struct mutex vendor_req_mutex;
+   void *vend_buf;
+
struct mutex reg_atomic_mutex;
struct mutex hw_atomic_mutex;
 
diff --git a/drivers/net/wireless/mediatek/mt7601u/usb.c 
b/drivers/net/wireless/mediatek/mt7601u/usb.c
index 54dba4001865..416c6045ff31 100644
--- a/drivers/net/wireless/mediatek/mt7601u/usb.c
+++ b/drivers/net/wireless/mediatek/mt7601u/usb.c
@@ -92,10 +92,9 @@ void mt7601u_complete_urb(struct urb *urb)
complete(cmpl);
 }
 
-static int
-__mt7601u_vendor_request(struct mt7601u_dev *dev, const u8 req,
-const u8 direction, const u16 val, const u16 offset,
-void *buf, const size_t buflen)
+int mt7601u_vendor_request(struct mt7601u_dev *dev, const u8 req,
+  const u8 direction, const u16 val, const u16 offset,
+  void *buf, const size_t buflen)
 {
int i, ret;
struct usb_device *usb_dev = mt7601u_to_usb_dev(dev);
@@ -110,6 +109,8 @@ __mt7601u_vendor_request(struct mt7601u_dev *dev, const u8 
req,
trace_mt_vend_req(dev, pipe, req, req_type, val, offset,
  buf, buflen, ret);
 
+   if (ret == -ENODEV)
+   set_bit(MT7601U_STATE_REMOVED, dev-state);
if (ret = 0 || ret == -ENODEV)
return ret;
 
@@ -122,25 +123,6 @@ __mt7601u_vendor_request(struct mt7601u_dev *dev, const u8 
req,
return ret;
 }
 
-int
-mt7601u_vendor_request(struct mt7601u_dev *dev, const u8 req,
-  const u8 direction, const u16 val, const u16 offset,
-  void *buf, const size_t buflen)
-{
-   int ret;
-
-   mutex_lock(dev-vendor_req_mutex);
-
-   ret = __mt7601u_vendor_request(dev, req, direction, val, offset,
-  buf, buflen);
-   if (ret == -ENODEV)
-   set_bit(MT7601U_STATE_REMOVED, dev-state);
-
-   mutex_unlock(dev-vendor_req_mutex);
-
-   return ret;
-}
-
 void mt7601u_vendor_reset(struct mt7601u_dev *dev)
 {
mt7601u_vendor_request(dev, MT_VEND_DEV_MODE, USB_DIR_OUT,
@@ -150,19 +132,21 @@ void mt7601u_vendor_reset(struct mt7601u_dev *dev)
 u32 mt7601u_rr(struct mt7601u_dev *dev, u32 offset)
 {
int ret;
-   __le32 reg;
-   u32 val;
+   u32 val = ~0;
 
WARN_ONCE(offset  USHRT_MAX, read high off:%08x, offset);
 
+   mutex_lock(dev-vendor_req_mutex);
+
ret = mt7601u_vendor_request(dev, MT_VEND_MULTI_READ, USB_DIR_IN,
-0, offset, reg, sizeof(reg));
-   val = le32_to_cpu(reg);
-   if (ret  0  ret != sizeof(reg)) {
+0, offset, dev-vend_buf, MT_VEND_BUF);
+   if (ret == MT_VEND_BUF)
+   val = get_unaligned_le32(dev-vend_buf);
+   else if (ret  0)
dev_err(dev-dev, Error: wrong size read:%d off:%08x\n,
ret, offset);
-   val = ~0;
-   }
+
+   mutex_unlock(dev-vendor_req_mutex);
 
trace_reg_read(dev, offset, val);
return val;
@@ -173,12 +157,17 @@ int mt7601u_vendor_single_wr(struct mt7601u_dev *dev, 
const u8 req,
 {
int ret;
 
+   mutex_lock(dev-vendor_req_mutex);
+
ret = mt7601u_vendor_request(dev, req, USB_DIR_OUT,
 val  0x, offset, NULL, 0);
-   if (ret)
-   return ret;
-   return mt7601u_vendor_request(dev, req

Re: Driver for Mediatek MT7630E

[Jakub Kicinski]

On Wed, 29 Jul 2015 09:38:04 +0200, Linus Walleij wrote:
 On Wed, Jul 29, 2015 at 1:24 AM, Xose Vazquez Perez
 xose.vazq...@gmail.com wrote:
  Linus Walleij wrote:
 
  Did anything ever happen to this?
 
  My daughter has this device in her laptop it seems, sigh.
 
  I might start fiddling with it unless someone else is already
  doing it.
 
  Jakub Kicinski was working on it at: https://github.com/kuba-moo/mt7630e
 
 Ah yeah that is what I have running. It's a fork of rt2x00 so it's
 missing all fixes since 3.11 or so, just a question of when this
 becomes unsupportable.

The rt2x00 does not change that much itself so it's not a biggie.
However, we are missing all the mac80211-related changes :/
 
 This code is a complete mashup of Linux and duct-taped Windows
 NDIS-based driver code, my eyes are bleeding from looking at it.
 
 The defines etc patched in have obvious gaps due to other unsupported
 chips such as MT7601u I suspect. I guess it's necessary to also
 look at this in order to not screw things up for 7601.
 https://github.com/porjo/mt7601

The problem with this hardware is that it's a something between old
Ralink stuff and new AC devices which Felix is supporting in mt76,
just like mt7601u.  I started hacking on mt76 to add support but not
sure if Felix is interested in merging support for old chips there.

So the support for mt7630e could be added in three places,
theoretically: (1) rt2x00, (2) mt7601u, (3) mt76.  IMO they're all bad
choices.

Also MediaTek has no interest in supporting Open Source driver for this
device.
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mt7601u: don't warn about devices without per-rate power table

[Jakub Kicinski]

From: Jakub Kicinski kubak...@wp.pl

We expect EEPROM per-rate power table to be filled with
s6 values and warn user if values are invalid.  However,
there appear to be devices which don't have this section
of EEPROM initialized.  In such case we should ignore
the values and leave the driver power tables set to zero.

Note that vendor driver doesn't care about this case but
mt76x2 skips 0xff per value.  We take mt76x2's approach.

Signed-off-by: Jakub Kicinski kubak...@wp.pl
---
Kalle, I tried my best with patchwork settings but if my name still
contains a question mark on this submission you can go ahead and
apply automatically anyway.  The patch itself does not have any hairy
characters and Johannes said on IRC yesterday that pwclient ends up
doing the right thing in this case.
---
drivers/net/wireless/mediatek/mt7601u/eeprom.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/mediatek/mt7601u/eeprom.c 
b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
index ce3837f270f0..8d8ee0344f7b 100644
--- a/drivers/net/wireless/mediatek/mt7601u/eeprom.c
+++ b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
@@ -277,6 +277,10 @@ mt7601u_extra_power_over_mac(struct mt7601u_dev *dev)
 static void
 mt7601u_set_power_rate(struct power_per_rate *rate, s8 delta, u8 value)
 {
+   /* Invalid? Note: vendor driver does not handle this */
+   if (value == 0xff)
+   return;
+
rate-raw = s6_validate(value);
rate-bw20 = s6_to_int(value);
/* Note: vendor driver does cap the value to s6 right away */
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] mt7601u: unify paged and non-paged RX dma paths

[Jakub Kicinski]

From: Jakub Kicinski kubak...@wp.pl

Signed-off-by: Jakub Kicinski kubak...@wp.pl
---
 drivers/net/wireless/mediatek/mt7601u/dma.c | 62 ++---
 1 file changed, 12 insertions(+), 50 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.c 
b/drivers/net/wireless/mediatek/mt7601u/dma.c
index 9c9e1288644b..16df67b2e62c 100644
--- a/drivers/net/wireless/mediatek/mt7601u/dma.c
+++ b/drivers/net/wireless/mediatek/mt7601u/dma.c
@@ -34,56 +34,28 @@ static unsigned int ieee80211_get_hdrlen_from_buf(const u8 
*data, unsigned len)
 
 static struct sk_buff *
 mt7601u_rx_skb_from_seg(struct mt7601u_dev *dev, struct mt7601u_rxwi *rxwi,
-   u8 *data, u32 seg_len)
+   void *data, u32 seg_len, u32 truesize, struct page *p)
 {
struct sk_buff *skb;
u32 true_len;
+   int hdr_len, copy, frag;
 
-   if (rxwi-rxinfo  cpu_to_le32(MT_RXINFO_L2PAD))
-   seg_len -= 2;
-
-   skb = alloc_skb(seg_len, GFP_ATOMIC);
-   if (!skb)
-   return NULL;
-
-   if (rxwi-rxinfo  cpu_to_le32(MT_RXINFO_L2PAD)) {
-   int hdr_len = ieee80211_get_hdrlen_from_buf(data, seg_len);
-
-   memcpy(skb_put(skb, hdr_len), data, hdr_len);
-   data += hdr_len + 2;
-   seg_len -= hdr_len;
-   }
-
-   memcpy(skb_put(skb, seg_len), data, seg_len);
-
-   true_len = mt76_mac_process_rx(dev, skb, skb-data, rxwi);
-   skb_trim(skb, true_len);
-
-   return skb;
-}
-
-static struct sk_buff *
-mt7601u_rx_skb_from_seg_paged(struct mt7601u_dev *dev,
- struct mt7601u_rxwi *rxwi, void *data,
- u32 seg_len, u32 truesize, struct page *p)
-{
-   unsigned int hdr_len = ieee80211_get_hdrlen_from_buf(data, seg_len);
-   unsigned int true_len, copy, frag;
-   struct sk_buff *skb;
-
-   skb = alloc_skb(128, GFP_ATOMIC);
+   skb = alloc_skb(p ? 128 : seg_len, GFP_ATOMIC);
if (!skb)
return NULL;
 
true_len = mt76_mac_process_rx(dev, skb, data, rxwi);
 
+   hdr_len = ieee80211_get_hdrlen_from_buf(data, true_len);
if (rxwi-rxinfo  cpu_to_le32(MT_RXINFO_L2PAD)) {
memcpy(skb_put(skb, hdr_len), data, hdr_len);
+
data += hdr_len + 2;
true_len -= hdr_len;
hdr_len = 0;
}
 
+   /* If not doing paged RX allocated skb will always have enough space */
copy = (true_len = skb_tailroom(skb)) ? true_len : hdr_len + 8;
frag = true_len - copy;
 
@@ -100,7 +72,7 @@ mt7601u_rx_skb_from_seg_paged(struct mt7601u_dev *dev,
 }
 
 static void mt7601u_rx_process_seg(struct mt7601u_dev *dev, u8 *data,
-  u32 seg_len, struct page *p, bool paged)
+  u32 seg_len, struct page *p)
 {
struct sk_buff *skb;
struct mt7601u_rxwi *rxwi;
@@ -126,11 +98,7 @@ static void mt7601u_rx_process_seg(struct mt7601u_dev *dev, 
u8 *data,
 
trace_mt_rx(dev, rxwi, fce_info);
 
-   if (paged)
-   skb = mt7601u_rx_skb_from_seg_paged(dev, rxwi, data, seg_len,
-   truesize, p);
-   else
-   skb = mt7601u_rx_skb_from_seg(dev, rxwi, data, seg_len);
+   skb = mt7601u_rx_skb_from_seg(dev, rxwi, data, seg_len, truesize, p);
if (!skb)
return;
 
@@ -158,23 +126,17 @@ mt7601u_rx_process_entry(struct mt7601u_dev *dev, struct 
mt7601u_dma_buf_rx *e)
u32 seg_len, data_len = e-urb-actual_length;
u8 *data = page_address(e-p);
struct page *new_p = NULL;
-   bool paged = true;
int cnt = 0;
 
if (!test_bit(MT7601U_STATE_INITIALIZED, dev-state))
return;
 
/* Copy if there is very little data in the buffer. */
-   if (data_len  512) {
-   paged = false;
-   } else {
+   if (data_len  512)
new_p = dev_alloc_pages(MT_RX_ORDER);
-   if (!new_p)
-   paged = false;
-   }
 
while ((seg_len = mt7601u_rx_next_seg_len(data, data_len))) {
-   mt7601u_rx_process_seg(dev, data, seg_len, e-p, paged);
+   mt7601u_rx_process_seg(dev, data, seg_len, new_p ? e-p : NULL);
 
data_len -= seg_len;
data += seg_len;
@@ -182,9 +144,9 @@ mt7601u_rx_process_entry(struct mt7601u_dev *dev, struct 
mt7601u_dma_buf_rx *e)
}
 
if (cnt  1)
-   trace_mt_rx_dma_aggr(dev, cnt, paged);
+   trace_mt_rx_dma_aggr(dev, cnt, !!new_p);
 
-   if (paged) {
+   if (new_p) {
/* we have one extra ref from the allocator */
__free_pages(e-p, MT_RX_ORDER);
 
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http

[PATCH 3/4] mt7601u: don't cleanup device second time after .resume()

[Jakub Kicinski]

From: Jakub Kicinski kubak...@wp.pl

Make sure .disconnect() doesn't cleanup the device if
.resume() failed.  This may happen when device is removed
during suspend.

Signed-off-by: Jakub Kicinski kubak...@wp.pl
---
 drivers/net/wireless/mediatek/mt7601u/init.c | 3 +++
 drivers/net/wireless/mediatek/mt7601u/usb.c  | 9 -
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt7601u/init.c 
b/drivers/net/wireless/mediatek/mt7601u/init.c
index 1fc86e865c8c..45eb0796a2e5 100644
--- a/drivers/net/wireless/mediatek/mt7601u/init.c
+++ b/drivers/net/wireless/mediatek/mt7601u/init.c
@@ -427,6 +427,9 @@ err:
 
 void mt7601u_cleanup(struct mt7601u_dev *dev)
 {
+   if (!test_and_clear_bit(MT7601U_STATE_INITIALIZED, dev-state))
+   return;
+
mt7601u_stop_hardware(dev);
mt7601u_dma_cleanup(dev);
mt7601u_mcu_cmd_deinit(dev);
diff --git a/drivers/net/wireless/mediatek/mt7601u/usb.c 
b/drivers/net/wireless/mediatek/mt7601u/usb.c
index 99e2b3997bfa..54dba4001865 100644
--- a/drivers/net/wireless/mediatek/mt7601u/usb.c
+++ b/drivers/net/wireless/mediatek/mt7601u/usb.c
@@ -338,8 +338,15 @@ static int mt7601u_suspend(struct usb_interface *usb_intf, 
pm_message_t state)
 static int mt7601u_resume(struct usb_interface *usb_intf)
 {
struct mt7601u_dev *dev = usb_get_intfdata(usb_intf);
+   int ret;
+
+   ret = mt7601u_init_hardware(dev);
+   if (ret)
+   return ret;
+
+   set_bit(MT7601U_STATE_INITIALIZED, dev-state);
 
-   return mt7601u_init_hardware(dev);
+   return 0;
 }
 
 MODULE_DEVICE_TABLE(usb, mt7601u_device_table);
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] mt7601u: watch out for invalid-length frames

[Jakub Kicinski]

From: Jakub Kicinski kubak...@wp.pl

Users of older Ralink devices report that received frames
sometimes have zero length.  Watch out for that.

Signed-off-by: Jakub Kicinski kubak...@wp.pl
---
 drivers/net/wireless/mediatek/mt7601u/dma.c | 14 --
 drivers/net/wireless/mediatek/mt7601u/mac.c |  8 ++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.c 
b/drivers/net/wireless/mediatek/mt7601u/dma.c
index 16df67b2e62c..7217da4f1543 100644
--- a/drivers/net/wireless/mediatek/mt7601u/dma.c
+++ b/drivers/net/wireless/mediatek/mt7601u/dma.c
@@ -37,16 +37,20 @@ mt7601u_rx_skb_from_seg(struct mt7601u_dev *dev, struct 
mt7601u_rxwi *rxwi,
void *data, u32 seg_len, u32 truesize, struct page *p)
 {
struct sk_buff *skb;
-   u32 true_len;
-   int hdr_len, copy, frag;
+   u32 true_len, hdr_len = 0, copy, frag;
 
skb = alloc_skb(p ? 128 : seg_len, GFP_ATOMIC);
if (!skb)
return NULL;
 
true_len = mt76_mac_process_rx(dev, skb, data, rxwi);
+   if (!true_len || true_len  seg_len)
+   goto bad_frame;
 
hdr_len = ieee80211_get_hdrlen_from_buf(data, true_len);
+   if (!hdr_len)
+   goto bad_frame;
+
if (rxwi-rxinfo  cpu_to_le32(MT_RXINFO_L2PAD)) {
memcpy(skb_put(skb, hdr_len), data, hdr_len);
 
@@ -69,6 +73,12 @@ mt7601u_rx_skb_from_seg(struct mt7601u_dev *dev, struct 
mt7601u_rxwi *rxwi,
}
 
return skb;
+
+bad_frame:
+   dev_err_ratelimited(dev-dev, Error: incorrect frame len:%u hdr:%u\n,
+   true_len, hdr_len);
+   dev_kfree_skb(skb);
+   return NULL;
 }
 
 static void mt7601u_rx_process_seg(struct mt7601u_dev *dev, u8 *data,
diff --git a/drivers/net/wireless/mediatek/mt7601u/mac.c 
b/drivers/net/wireless/mediatek/mt7601u/mac.c
index c161bcc6a7fa..7514bce1ac91 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mac.c
+++ b/drivers/net/wireless/mediatek/mt7601u/mac.c
@@ -450,10 +450,14 @@ u32 mt76_mac_process_rx(struct mt7601u_dev *dev, struct 
sk_buff *skb,
 {
struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
struct mt7601u_rxwi *rxwi = rxi;
-   u32 ctl = le32_to_cpu(rxwi-ctl);
+   u32 len, ctl = le32_to_cpu(rxwi-ctl);
u16 rate = le16_to_cpu(rxwi-rate);
int rssi;
 
+   len = MT76_GET(MT_RXWI_CTL_MPDU_LEN, ctl);
+   if (len  10)
+   return 0;
+
if (rxwi-rxinfo  cpu_to_le32(MT_RXINFO_DECRYPT)) {
status-flag |= RX_FLAG_DECRYPTED;
status-flag |= RX_FLAG_IV_STRIPPED | RX_FLAG_MMIC_STRIPPED;
@@ -474,7 +478,7 @@ u32 mt76_mac_process_rx(struct mt7601u_dev *dev, struct 
sk_buff *skb,
dev-avg_rssi = (dev-avg_rssi * 15) / 16 + (rssi  8);
spin_unlock_bh(dev-con_mon_lock);
 
-   return MT76_GET(MT_RXWI_CTL_MPDU_LEN, ctl);
+   return len;
 }
 
 static enum mt76_cipher_type
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] mt7601u: set promiscous mode based on FIF_OTHER_BSS

[Jakub Kicinski]

From: Jakub Kicinski kubak...@wp.pl

Most drivers use FIF_OTHER_BSS to set promiscous mode.  Let us
follow their lead even though it doesn't match exactly the HW
filter flags.

Signed-off-by: Jakub Kicinski kubak...@wp.pl
---
 drivers/net/wireless/mediatek/mt7601u/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/mediatek/mt7601u/main.c 
b/drivers/net/wireless/mediatek/mt7601u/main.c
index ced82abb414f..169384b48b27 100644
--- a/drivers/net/wireless/mediatek/mt7601u/main.c
+++ b/drivers/net/wireless/mediatek/mt7601u/main.c
@@ -119,6 +119,7 @@ mt76_configure_filter(struct ieee80211_hw *hw, unsigned int 
changed_flags,
 
dev-rxfilter = ~MT_RX_FILTR_CFG_OTHER_BSS;
 
+   MT76_FILTER(OTHER_BSS, MT_RX_FILTR_CFG_PROMISC);
MT76_FILTER(FCSFAIL, MT_RX_FILTR_CFG_CRC_ERR);
MT76_FILTER(PLCPFAIL, MT_RX_FILTR_CFG_PHY_ERR);
MT76_FILTER(CONTROL, MT_RX_FILTR_CFG_ACK |
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mac80211: remove obsolete sentence from documentation

[Jakub Kicinski]

From: Jakub Kicinski kubak...@wp.pl

FIF_PROMISC_IN_BSS was removed in commit df1404650ccb
(mac80211: remove support for IFF_PROMISC).

Signed-off-by: Jakub Kicinski kubak...@wp.pl
---
 include/net/mac80211.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 2f8b7decace0..e117119927ec 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2591,8 +2591,7 @@ void ieee80211_free_txskb(struct ieee80211_hw *hw, struct 
sk_buff *skb);
  *
  * @FIF_OTHER_BSS: pass frames destined to other BSSes
  *
- * @FIF_PSPOLL: pass PS Poll frames, if PROMISC_IN_BSS is not set then only
- * those addressed to this station.
+ * @FIF_PSPOLL: pass PS Poll frames
  *
  * @FIF_PROBE_REQ: pass probe request frames
  */
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv3 2/2] add mt7601u kbuild and others

[Jakub Kicinski]

From: Jakub Kicinski kubak...@wp.pl

Signed-off-by: Jakub Kicinski kubak...@wp.pl
---
 MAINTAINERS|  6 ++
 drivers/net/wireless/Kconfig   |  1 +
 drivers/net/wireless/Makefile  |  2 ++
 drivers/net/wireless/mediatek/Kconfig  | 10 ++
 drivers/net/wireless/mediatek/Makefile |  1 +
 drivers/net/wireless/mediatek/mt7601u/Kconfig  |  6 ++
 drivers/net/wireless/mediatek/mt7601u/Makefile |  9 +
 7 files changed, 35 insertions(+)
 create mode 100644 drivers/net/wireless/mediatek/Kconfig
 create mode 100644 drivers/net/wireless/mediatek/Makefile
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/Kconfig
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/Makefile

diff --git a/MAINTAINERS b/MAINTAINERS
index df106f87a3ba..c6f4576efd56 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6365,6 +6365,12 @@ F:   include/uapi/linux/meye.h
 F: include/uapi/linux/ivtv*
 F: include/uapi/linux/uvcvideo.h
 
+MEDIATEK MT7601U WIRELESS LAN DRIVER
+M: Jakub Kicinski kubak...@wp.pl
+L: linux-wireless@vger.kernel.org
+S: Maintained
+F: drivers/net/wireless/mediatek/mt7601u/
+
 MEGARAID SCSI/SAS DRIVERS
 M: Kashyap Desai kashyap.de...@avagotech.com
 M: Sumit Saxena sumit.sax...@avagotech.com
diff --git a/drivers/net/wireless/Kconfig b/drivers/net/wireless/Kconfig
index 16604bdf5197..a63ab2e83105 100644
--- a/drivers/net/wireless/Kconfig
+++ b/drivers/net/wireless/Kconfig
@@ -277,6 +277,7 @@ source drivers/net/wireless/libertas/Kconfig
 source drivers/net/wireless/orinoco/Kconfig
 source drivers/net/wireless/p54/Kconfig
 source drivers/net/wireless/rt2x00/Kconfig
+source drivers/net/wireless/mediatek/Kconfig
 source drivers/net/wireless/rtlwifi/Kconfig
 source drivers/net/wireless/ti/Kconfig
 source drivers/net/wireless/zd1211rw/Kconfig
diff --git a/drivers/net/wireless/Makefile b/drivers/net/wireless/Makefile
index 0c8891686718..6b9e729dd8ac 100644
--- a/drivers/net/wireless/Makefile
+++ b/drivers/net/wireless/Makefile
@@ -45,6 +45,8 @@ obj-$(CONFIG_IWLWIFI) += iwlwifi/
 obj-$(CONFIG_IWLEGACY) += iwlegacy/
 obj-$(CONFIG_RT2X00)   += rt2x00/
 
+obj-$(CONFIG_WL_MEDIATEK)  += mediatek/
+
 obj-$(CONFIG_P54_COMMON)   += p54/
 
 obj-$(CONFIG_ATH_CARDS)+= ath/
diff --git a/drivers/net/wireless/mediatek/Kconfig 
b/drivers/net/wireless/mediatek/Kconfig
new file mode 100644
index ..cba300c6b5da
--- /dev/null
+++ b/drivers/net/wireless/mediatek/Kconfig
@@ -0,0 +1,10 @@
+menuconfig WL_MEDIATEK
+   bool Mediatek Wireless LAN support
+   ---help---
+ Enable community drivers for MediaTek WiFi devices.
+ Those drivers make use of the Linux mac80211 stack.
+
+
+if WL_MEDIATEK
+source drivers/net/wireless/mediatek/mt7601u/Kconfig
+endif # WL_MEDIATEK
diff --git a/drivers/net/wireless/mediatek/Makefile 
b/drivers/net/wireless/mediatek/Makefile
new file mode 100644
index ..9d5f182fd7fd
--- /dev/null
+++ b/drivers/net/wireless/mediatek/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_MT7601U)  += mt7601u/
diff --git a/drivers/net/wireless/mediatek/mt7601u/Kconfig 
b/drivers/net/wireless/mediatek/mt7601u/Kconfig
new file mode 100644
index ..f46bed92796b
--- /dev/null
+++ b/drivers/net/wireless/mediatek/mt7601u/Kconfig
@@ -0,0 +1,6 @@
+config MT7601U
+   tristate MediaTek MT7601U (USB) support
+   depends on MAC80211
+   depends on USB
+   ---help---
+ This adds support for MT7601U-based wireless USB dongles.
diff --git a/drivers/net/wireless/mediatek/mt7601u/Makefile 
b/drivers/net/wireless/mediatek/mt7601u/Makefile
new file mode 100644
index ..ea9ed8a5db4d
--- /dev/null
+++ b/drivers/net/wireless/mediatek/mt7601u/Makefile
@@ -0,0 +1,9 @@
+ccflags-y += -D__CHECK_ENDIAN__
+
+obj-$(CONFIG_MT7601U)  += mt7601u.o
+
+mt7601u-objs   = \
+   usb.o init.o main.o mcu.o trace.o dma.o core.o eeprom.o phy.o \
+   mac.o util.o debugfs.o tx.o
+
+CFLAGS_trace.o := -I$(src)
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv3 0/2] add mt7601u driver

[Jakub Kicinski]

From: Jakub Kicinski kubak...@wp.pl

 This miniseries adds support for the simplest of MediaTek Wi-Fi
 devices.  MT7601U is a single stream bgn chip with no bells or whistles.
 My driver is partially based on Felix's mt76 but IMHO it doesn't
 make sense to merge the two right now because MT7601U is a design
 somewhere between old Ralink devices and new Mediatek chips.  There
 wouldn't be all that much code sharing with the devices mt76 supports.
 Situation may obviously change when someone decides to extend m76 with
 support for the more recent USB dongles.
 
 The driver supports only station mode.  I'm hoping to add AP support
 when time allows.
 
 This driver sat on GitHub for quite a while and got some testing there.
 If anyone is interested in full git history and such here's a link:
 
 http://github.com/kuba-moo/mt7601u
 
 I split the submission into the build things and meta data (kconfig,
 Makefiles, MAINTAINERS update etc.) and the actual code to make the
 reviewing a little easier.

v2:
 - don't zero parts of just allocated skb (Johannes)
 - add delay to polling of MAC state (Johannes)
 - use paged RX (Johannes)
 - add more device IDs (Jose)
 - reduce number of RX buffers
 - increase max length of USB dma aggregate

v3:
 - rebase

Jakub Kicinski (2):
  add mt7601u driver
  add mt7601u kbuild and others

 MAINTAINERS|6 +
 drivers/net/wireless/Kconfig   |1 +
 drivers/net/wireless/Makefile  |2 +
 drivers/net/wireless/mediatek/Kconfig  |   10 +
 drivers/net/wireless/mediatek/Makefile |1 +
 drivers/net/wireless/mediatek/mt7601u/Kconfig  |6 +
 drivers/net/wireless/mediatek/mt7601u/Makefile |9 +
 drivers/net/wireless/mediatek/mt7601u/core.c   |   78 ++
 drivers/net/wireless/mediatek/mt7601u/debugfs.c|  172 +++
 drivers/net/wireless/mediatek/mt7601u/dma.c|  533 +
 drivers/net/wireless/mediatek/mt7601u/dma.h|  127 ++
 drivers/net/wireless/mediatek/mt7601u/eeprom.c |  414 +++
 drivers/net/wireless/mediatek/mt7601u/eeprom.h |  151 +++
 drivers/net/wireless/mediatek/mt7601u/init.c   |  625 ++
 drivers/net/wireless/mediatek/mt7601u/initvals.h   |  164 +++
 .../net/wireless/mediatek/mt7601u/initvals_phy.h   |  291 +
 drivers/net/wireless/mediatek/mt7601u/mac.c|  569 +
 drivers/net/wireless/mediatek/mt7601u/mac.h|  178 +++
 drivers/net/wireless/mediatek/mt7601u/main.c   |  412 +++
 drivers/net/wireless/mediatek/mt7601u/mcu.c|  534 +
 drivers/net/wireless/mediatek/mt7601u/mcu.h|   94 ++
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h|  390 ++
 drivers/net/wireless/mediatek/mt7601u/phy.c| 1251 
 drivers/net/wireless/mediatek/mt7601u/regs.h   |  636 ++
 drivers/net/wireless/mediatek/mt7601u/trace.c  |   21 +
 drivers/net/wireless/mediatek/mt7601u/trace.h  |  400 +++
 drivers/net/wireless/mediatek/mt7601u/tx.c |  319 +
 drivers/net/wireless/mediatek/mt7601u/usb.c|  360 ++
 drivers/net/wireless/mediatek/mt7601u/usb.h|   77 ++
 drivers/net/wireless/mediatek/mt7601u/util.c   |   42 +
 drivers/net/wireless/mediatek/mt7601u/util.h   |   77 ++
 31 files changed, 7950 insertions(+)
 create mode 100644 drivers/net/wireless/mediatek/Kconfig
 create mode 100644 drivers/net/wireless/mediatek/Makefile
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/Kconfig
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/Makefile
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/core.c
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/debugfs.c
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/dma.c
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/dma.h
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/eeprom.c
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/eeprom.h
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/init.c
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/initvals.h
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/initvals_phy.h
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/mac.c
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/mac.h
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/main.c
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/mcu.c
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/mcu.h
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/mt7601u.h
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/phy.c
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/regs.h
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/trace.c
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/trace.h
 create mode 100644 drivers/net/wireless/mediatek/mt7601u/tx.c
 create mode 100644 drivers/net/wireless/mediatek/mt7601u

Re: [rt2x00-users] Ralink RT5390/RT5370 no longer works on more recent kernels

[Jakub Kicinski]

Hello Simon,

On Sat, 7 Feb 2015 17:54:15 +0100, Simon Raffeiner (SCC) wrote:
 Hello everyone,
 
 one of my USB wireless adapters, based on the Ralink RT5390/RT5370
 chipset and RF, no longer works on more recent kernels. It doesn't
 report any networks on iw dev wlan0 scan and doesn't associate if told
 to do so.

I tested a TP-Link TL-727N v3.2 which also is:

RT chipset 5390, rev 0502 detected
RF chipset 5370 detected

and it works fine on 3.19-rc5 (from the wireless-testing tree).

Are you testing the different kernels on the same machine?  
Can you try bisection?


kuba
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html