Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1429679 > unrolled thread

Re: [PATCH] wlcore: time sync : add support for 64 bit clock

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] wlcore: time sync : add support for 64 bit clock Johannes Berg <johannes@sipsolutions.net> - 2016-06-23 13:20 +0200
    RE: [PATCH] wlcore: time sync : add support for 64 bit clock "Machani, Yaniv" <yanivma@ti.com> - 2016-06-23 13:40 +0200
      Re: [PATCH] wlcore: time sync : add support for 64 bit clock Johannes Berg <johannes@sipsolutions.net> - 2016-06-23 13:40 +0200

#1429679 — Re: [PATCH] wlcore: time sync : add support for 64 bit clock

On Thu, 2016-06-23 at 14:12 +0300, Yaniv Machani wrote:
> Changed the configuration to support 64bit instead of 32bit
> this in order to offload the driver from handling a wraparound.

[...]

Since you Cc'ed me, and presumably want me to review it, I'll say that
this looks like a terrible idea:

> @@ -74,10 +74,16 @@ struct wl18xx_event_mailbox {

This struct is evidently used for firmware/host communication.

>  	__le16 bss_loss_bitmap;
>  
>  	/* bitmap of stations (by HLID) which exceeded max tx
> retries */
> -	__le32 tx_retry_exceeded_bitmap;
> +	__le16 tx_retry_exceeded_bitmap;
> +
> +	/* time sync high msb*/
> +	u16 time_sync_tsf_high_msb;

So first of all, just using u16 instead of __le16 seems wrong.

Additionally, this looks like it changes the firmware API, so that
older firmware images will no longer work?

johannes

[toc] | [next] | [standalone]

#1429710

On Thu, Jun 23, 2016 at 14:18:00, Johannes Berg wrote:
> linux-wireless@vger.kernel.org; netdev@vger.kernel.org
> Subject: Re: [PATCH] wlcore: time sync : add support for 64 bit clock
> 
> On Thu, 2016-06-23 at 14:12 +0300, Yaniv Machani wrote:
> > Changed the configuration to support 64bit instead of 32bit this in 
> > order to offload the driver from handling a wraparound.
> 
> [...]
> 
> Since you Cc'ed me, and presumably want me to review it, I'll say that 
> this looks like a terrible idea:
> 
> > @@ -74,10 +74,16 @@ struct wl18xx_event_mailbox {
> 
> This struct is evidently used for firmware/host communication.
> 
> >  	__le16 bss_loss_bitmap;
> >
> >  	/* bitmap of stations (by HLID) which exceeded max tx retries */
> > -	__le32 tx_retry_exceeded_bitmap;
> > +	__le16 tx_retry_exceeded_bitmap;
> > +
> > +	/* time sync high msb*/
> > +	u16 time_sync_tsf_high_msb;
> 
> So first of all, just using u16 instead of __le16 seems wrong.

Agree, should be changed.

> 
> Additionally, this looks like it changes the firmware API, so that 
> older firmware images will no longer work?

It is backwards compatible, 
although it changes a API structure, older firmware are using only u16 for the field so there is no impact on that.
Of course that for actually using the 64bit information, you will have to upgrade the firmware.

Yaniv

[toc] | [prev] | [next] | [standalone]

#1429711

> > Additionally, this looks like it changes the firmware API, so that 
> > older firmware images will no longer work?
> 
> It is backwards compatible, 
> although it changes a API structure, older firmware are using only
> u16 for the field so there is no impact on that.
> 

Oh, ok. I had also thought that the size changed, but missed that you
replaced a u32 with two u16. Thanks for checking :)

johannes

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web