Re: [PATCH 1/2] staging: net: wireless: add ESP8089 WiFi driver

[Kalle Valo]

(adding linux-wireless)

Quentin Schulz  writes:

> Hi Marcel,
>
> On 21/07/2017 18:52, Marcel Holtmann wrote:
>> Hi Quentin,
>> 
> The Espressif ESP8089 WiFi chips can be often found in cheap tablets.
> There is one in A23 Polaroid tablets for example.
>
> The chip is often embedded as an eMMC SDIO device.
>
> The code was taken from an out-of-tree repository and has seen a first
> pass in the cleanup process.
>
> At the moment, there is no publicly available datasheet for this chip.
>
> Cc: Hans de Goede 
> Cc: Icenowy Zheng 
> Signed-off-by: Quentin Schulz 

 Staging drivers need a TODO file that lists what has to be done to the
 code to get it out of staging.  Why not just take a day or so and fix up
 the remaining issues and get it into the "real" part of the kernel
 correctly?

>>>
>>> OK, I'll work on a TODO list. Is there anything else I should know about
>>> staging drivers so I can address everything at the same time?
>>>
>>> From a driver that has already been cleaned up a bit by Icenowy and
>>> Hans, it took me between 10 and 15 working days to this step, which I
>>> estimate to be around 50% of total clean up (and we're only speaking
>>> about coding style and dead code mainly, nothing about a bit of code
>>> review, code robustness...). I find the code not really easy to follow
>>> (might be because I'm a beginner in the subsystem as well).
>>>
>>> I might not be the most efficient person in cleaning up drivers but I'm
>>> pretty sure this isn't a one day cleanup. (Would be happy to be proven
>>> otherwise :) ), else I would have done it as you suggest.
>> 
>> even if it takes you 1 month to clean it up, get it reviewed on
>> linux-wireless and target wireless-drivers instead of staging. When I
>> had a brief a look at your patch, it didn't look like staging
>> material to me.

I did only a 30 sec review (right now no time for proper review because
I have quite a lot of catching up after vacation) but based on what I
saw the driver looks promising. So I agree with Marcel, you should try
to submit this via linux-wireless first and only use staging as the last
resort.

> We don't have a client supporting this effort and I don't think the
> company I work for would support this effort (maybe it would, but
> definitely spread over a long long period), so we're talking about 10-15
> working days spread over my free time/week-end, that isn't for in one
> month :) I could use help for sure on this driver, that's why I posted
> it in staging.
>
> I've done the cleanup on a per-file basis so maybe you looked at one of
> the cleaned up files?
>
> Just to be sure, you're telling me that I should post it as is on
> linux-wireless and then work with the reviews? Or are you telling me to
> take "1 month to clean it up" and then post it on linux-wireless?

I recommend to post the patch to linux-wireless now and see what
comments you get. Then I can also when a proper review and have better
guidance.

-- 
Kalle Valo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: net: wireless: add ESP8089 WiFi driver

[kbuild test robot]

Hi Quentin,

[auto build test WARNING on next-20170719]
[cannot apply to staging/staging-testing linus/master linux/master v4.13-rc1 
v4.12 v4.12-rc7 v4.13-rc1]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Quentin-Schulz/add-ESP8089-WiFi-chip-driver/20170723-143744
config: blackfin-allyesconfig (attached as .config)
compiler: bfin-uclinux-gcc (GCC) 6.2.0
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=blackfin 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/staging//esp8089/esp_sip.c: In function 'sip_txq_process':
>> drivers/staging//esp8089/esp_sip.c:797:32: warning: 'offset' may be used 
>> uninitialized in this function [-Wmaybe-uninitialized]
 memcpy(sip->tx_aggr_write_ptr + offset, skb->data, skb->len);
~~~^~~~
   drivers/staging//esp8089/esp_sip.c:700:14: note: 'offset' was declared here
 u32 tx_len, offset;
 ^~
   drivers/staging//esp8089/esp_sip.c: In function 'sip_poll_resetting_event':
>> drivers/staging//esp8089/esp_sip.c:1662:5: warning: 'ret' may be used 
>> uninitialized in this function [-Wmaybe-uninitialized]
 if (!ret) {
^
--
   drivers/staging//esp8089/esp_mac80211.c: In function 'esp_op_set_key':
>> drivers/staging//esp8089/esp_mac80211.c:529:6: warning: 'index' may be used 
>> uninitialized in this function [-Wmaybe-uninitialized]
  map[index].flag = 0;
 ^

vim +/offset +797 drivers/staging//esp8089/esp_sip.c

   691  
   692  /* setup sip header and tx info, copy pkt into aggr buf */
   693  static int sip_pack_pkt(struct esp_sip *sip, struct sk_buff *skb, int 
*pm_state)
   694  {
   695  struct ieee80211_tx_info *itx_info;
   696  struct sip_hdr *shdr;
   697  struct ieee80211_hdr *wh;
   698  struct esp_vif *evif;
   699  struct esp_node *node;
   700  u32 tx_len, offset;
   701  bool is_data = true;
   702  u8 sta_index;
   703  int alg;
   704  
   705  itx_info = IEEE80211_SKB_CB(skb);
   706  if (itx_info->flags == 0x) {
   707  shdr = (struct sip_hdr *)skb->data;
   708  is_data = false;
   709  tx_len = skb->len;
   710  } else {
   711  wh = (struct ieee80211_hdr *)skb->data;
   712  evif = (struct esp_vif 
*)itx_info->control.vif->drv_priv;
   713  /* update sip header */
   714  shdr = (struct sip_hdr *)sip->tx_aggr_write_ptr;
   715  
   716  shdr->fc[0] = 0;
   717  shdr->fc[1] = 0;
   718  
   719  if (itx_info->flags & IEEE80211_TX_CTL_AMPDU)
   720  SIP_HDR_SET_TYPE(shdr->fc[0], SIP_DATA_AMPDU);
   721  else
   722  SIP_HDR_SET_TYPE(shdr->fc[0], SIP_DATA);
   723  
   724  if (!evif->epub) {
   725  sip_tx_status_report(sip, skb, itx_info, false);
   726  atomic_dec(>tx_data_pkt_queued);
   727  return -EINVAL;
   728  }
   729  
   730  /* make room for encrypted pkt */
   731  if (itx_info->control.hw_key) {
   732  alg = 
esp_cipher2alg(itx_info->control.hw_key->cipher);
   733  if (unlikely(alg == -1)) {
   734  sip_tx_status_report(sip, skb, 
itx_info, false);
   735  atomic_dec(>tx_data_pkt_queued);
   736  return -1;
   737  }
   738  
   739  shdr->d_enc_flag = alg + 1;
   740  shdr->d_hw_kid = 
itx_info->control.hw_key->hw_key_idx |
   741  (evif->index << 7);
   742  } else {
   743  shdr->d_enc_flag = 0;
   744  shdr->d_hw_kid = evif->index << 7 | evif->index;
   745  }
   746  
   747  /* update sip tx info */
   748  node = esp_get_node_by_addr(sip->epub, wh->addr1);
   749  if (node)
   750  sta_index = node->index;
   751  else
   752  sta_index = ESP_PUB_MAX_STA + 1;
   753  
   754  SIP_HDR_SET_IFIDX(shdr->fc[0], evif->index << 3 | 
sta_index);
   755  shdr->d_p2p = itx_info->control.vif->p2p;
   756  
   757  

Re: [PATCH 1/2] staging: net: wireless: add ESP8089 WiFi driver

[Quentin Schulz]

Hi Marcel,

On 21/07/2017 18:52, Marcel Holtmann wrote:
> Hi Quentin,
> 
 The Espressif ESP8089 WiFi chips can be often found in cheap tablets.
 There is one in A23 Polaroid tablets for example.

 The chip is often embedded as an eMMC SDIO device.

 The code was taken from an out-of-tree repository and has seen a first
 pass in the cleanup process.

 At the moment, there is no publicly available datasheet for this chip.

 Cc: Hans de Goede 
 Cc: Icenowy Zheng 
 Signed-off-by: Quentin Schulz 
>>>
>>> Staging drivers need a TODO file that lists what has to be done to the
>>> code to get it out of staging.  Why not just take a day or so and fix up
>>> the remaining issues and get it into the "real" part of the kernel
>>> correctly?
>>>
>>
>> OK, I'll work on a TODO list. Is there anything else I should know about
>> staging drivers so I can address everything at the same time?
>>
>> From a driver that has already been cleaned up a bit by Icenowy and
>> Hans, it took me between 10 and 15 working days to this step, which I
>> estimate to be around 50% of total clean up (and we're only speaking
>> about coding style and dead code mainly, nothing about a bit of code
>> review, code robustness...). I find the code not really easy to follow
>> (might be because I'm a beginner in the subsystem as well).
>>
>> I might not be the most efficient person in cleaning up drivers but I'm
>> pretty sure this isn't a one day cleanup. (Would be happy to be proven
>> otherwise :) ), else I would have done it as you suggest.
> 
> even if it takes you 1 month to clean it up, get it reviewed on 
> linux-wireless and target wireless-drivers instead of staging. When I had a 
> brief a look at your patch, it didn't look like staging material to me.
> 

We don't have a client supporting this effort and I don't think the
company I work for would support this effort (maybe it would, but
definitely spread over a long long period), so we're talking about 10-15
working days spread over my free time/week-end, that isn't for in one
month :) I could use help for sure on this driver, that's why I posted
it in staging.

I've done the cleanup on a per-file basis so maybe you looked at one of
the cleaned up files?

Just to be sure, you're telling me that I should post it as is on
linux-wireless and then work with the reviews? Or are you telling me to
take "1 month to clean it up" and then post it on linux-wireless?

Thanks,
Quentin

> Regards
> 
> Marcel
> 

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: net: wireless: add ESP8089 WiFi driver

[Marcel Holtmann]

Hi Quentin,

>>> The Espressif ESP8089 WiFi chips can be often found in cheap tablets.
>>> There is one in A23 Polaroid tablets for example.
>>> 
>>> The chip is often embedded as an eMMC SDIO device.
>>> 
>>> The code was taken from an out-of-tree repository and has seen a first
>>> pass in the cleanup process.
>>> 
>>> At the moment, there is no publicly available datasheet for this chip.
>>> 
>>> Cc: Hans de Goede 
>>> Cc: Icenowy Zheng 
>>> Signed-off-by: Quentin Schulz 
>> 
>> Staging drivers need a TODO file that lists what has to be done to the
>> code to get it out of staging.  Why not just take a day or so and fix up
>> the remaining issues and get it into the "real" part of the kernel
>> correctly?
>> 
> 
> OK, I'll work on a TODO list. Is there anything else I should know about
> staging drivers so I can address everything at the same time?
> 
> From a driver that has already been cleaned up a bit by Icenowy and
> Hans, it took me between 10 and 15 working days to this step, which I
> estimate to be around 50% of total clean up (and we're only speaking
> about coding style and dead code mainly, nothing about a bit of code
> review, code robustness...). I find the code not really easy to follow
> (might be because I'm a beginner in the subsystem as well).
> 
> I might not be the most efficient person in cleaning up drivers but I'm
> pretty sure this isn't a one day cleanup. (Would be happy to be proven
> otherwise :) ), else I would have done it as you suggest.

even if it takes you 1 month to clean it up, get it reviewed on linux-wireless 
and target wireless-drivers instead of staging. When I had a brief a look at 
your patch, it didn't look like staging material to me.

Regards

Marcel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: net: wireless: add ESP8089 WiFi driver

[Quentin Schulz]

Hi Greg,

On 21/07/2017 17:01, Greg KH wrote:
> On Fri, Jul 21, 2017 at 04:35:01PM +0200, Quentin Schulz wrote:
>> The Espressif ESP8089 WiFi chips can be often found in cheap tablets.
>> There is one in A23 Polaroid tablets for example.
>>
>> The chip is often embedded as an eMMC SDIO device.
>>
>> The code was taken from an out-of-tree repository and has seen a first
>> pass in the cleanup process.
>>
>> At the moment, there is no publicly available datasheet for this chip.
>>
>> Cc: Hans de Goede 
>> Cc: Icenowy Zheng 
>> Signed-off-by: Quentin Schulz 
> 
> Staging drivers need a TODO file that lists what has to be done to the
> code to get it out of staging.  Why not just take a day or so and fix up
> the remaining issues and get it into the "real" part of the kernel
> correctly?
> 

OK, I'll work on a TODO list. Is there anything else I should know about
staging drivers so I can address everything at the same time?

>From a driver that has already been cleaned up a bit by Icenowy and
Hans, it took me between 10 and 15 working days to this step, which I
estimate to be around 50% of total clean up (and we're only speaking
about coding style and dead code mainly, nothing about a bit of code
review, code robustness...). I find the code not really easy to follow
(might be because I'm a beginner in the subsystem as well).

I might not be the most efficient person in cleaning up drivers but I'm
pretty sure this isn't a one day cleanup. (Would be happy to be proven
otherwise :) ), else I would have done it as you suggest.

> Also, staging drivers have to be "stand-alone", I can't take stuff that
> requires core changes only for one staging driver.
> 

Yes, I didn't expect the first version to go through, the goal was to
revive the discussion on this core patch as there was a consensus that
the requested feature was needed.

Thanks,
Quentin

> thanks,
> 
> greg k-h
> 

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: net: wireless: add ESP8089 WiFi driver

[Marcel Holtmann]

Hi Quentin,

> The Espressif ESP8089 WiFi chips can be often found in cheap tablets.
> There is one in A23 Polaroid tablets for example.
> 
> The chip is often embedded as an eMMC SDIO device.
> 
> The code was taken from an out-of-tree repository and has seen a first
> pass in the cleanup process.
> 
> At the moment, there is no publicly available datasheet for this chip.
> 
> Cc: Hans de Goede 
> Cc: Icenowy Zheng 
> Signed-off-by: Quentin Schulz 
> ---
> drivers/staging/Kconfig |2 +
> drivers/staging/Makefile|1 +
> drivers/staging/esp8089/Kconfig |   13 +
> drivers/staging/esp8089/Makefile|7 +
> drivers/staging/esp8089/esp_ctrl.c  |  527 
> drivers/staging/esp8089/esp_ctrl.h  |   48 +
> drivers/staging/esp8089/esp_debug.c |  247 
> drivers/staging/esp8089/esp_debug.h |   69 ++
> drivers/staging/esp8089/esp_file.c  |  221 
> drivers/staging/esp8089/esp_file.h  |   30 +
> drivers/staging/esp8089/esp_init_data.h |   17 +
> drivers/staging/esp8089/esp_io.c|  294 +
> drivers/staging/esp8089/esp_mac80211.c  | 1496 +++
> drivers/staging/esp8089/esp_mac80211.h  |   33 +
> drivers/staging/esp8089/esp_main.c  |  199 
> drivers/staging/esp8089/esp_pub.h   |  188 +++
> drivers/staging/esp8089/esp_sif.h   |  131 ++
> drivers/staging/esp8089/esp_sip.c   | 1718 +++
> drivers/staging/esp8089/esp_sip.h   |  150 +++
> drivers/staging/esp8089/esp_utils.c |  133 +++
> drivers/staging/esp8089/esp_utils.h |   27 +
> drivers/staging/esp8089/esp_wl.h|   35 +
> drivers/staging/esp8089/esp_wmac.h  |   87 ++
> drivers/staging/esp8089/sdio_sif_esp.c  |  552 +
> drivers/staging/esp8089/sip2_common.h   |  388 ++
> drivers/staging/esp8089/slc_host_register.h |  263 
> 26 files changed, 6876 insertions(+)
> create mode 100644 drivers/staging/esp8089/Kconfig
> create mode 100644 drivers/staging/esp8089/Makefile
> create mode 100644 drivers/staging/esp8089/esp_ctrl.c
> create mode 100644 drivers/staging/esp8089/esp_ctrl.h
> create mode 100644 drivers/staging/esp8089/esp_debug.c
> create mode 100644 drivers/staging/esp8089/esp_debug.h
> create mode 100644 drivers/staging/esp8089/esp_file.c
> create mode 100644 drivers/staging/esp8089/esp_file.h
> create mode 100644 drivers/staging/esp8089/esp_init_data.h
> create mode 100644 drivers/staging/esp8089/esp_io.c
> create mode 100644 drivers/staging/esp8089/esp_mac80211.c
> create mode 100644 drivers/staging/esp8089/esp_mac80211.h
> create mode 100644 drivers/staging/esp8089/esp_main.c
> create mode 100644 drivers/staging/esp8089/esp_pub.h
> create mode 100644 drivers/staging/esp8089/esp_sif.h
> create mode 100644 drivers/staging/esp8089/esp_sip.c
> create mode 100644 drivers/staging/esp8089/esp_sip.h
> create mode 100644 drivers/staging/esp8089/esp_utils.c
> create mode 100644 drivers/staging/esp8089/esp_utils.h
> create mode 100644 drivers/staging/esp8089/esp_wl.h
> create mode 100644 drivers/staging/esp8089/esp_wmac.h
> create mode 100644 drivers/staging/esp8089/sdio_sif_esp.c
> create mode 100644 drivers/staging/esp8089/sip2_common.h
> create mode 100644 drivers/staging/esp8089/slc_host_register.h

why are you putting this into staging? Is it that bad?

Regards

Marcel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: net: wireless: add ESP8089 WiFi driver

[Greg KH]

On Fri, Jul 21, 2017 at 04:35:01PM +0200, Quentin Schulz wrote:
> The Espressif ESP8089 WiFi chips can be often found in cheap tablets.
> There is one in A23 Polaroid tablets for example.
> 
> The chip is often embedded as an eMMC SDIO device.
> 
> The code was taken from an out-of-tree repository and has seen a first
> pass in the cleanup process.
> 
> At the moment, there is no publicly available datasheet for this chip.
> 
> Cc: Hans de Goede 
> Cc: Icenowy Zheng 
> Signed-off-by: Quentin Schulz 

Staging drivers need a TODO file that lists what has to be done to the
code to get it out of staging.  Why not just take a day or so and fix up
the remaining issues and get it into the "real" part of the kernel
correctly?

Also, staging drivers have to be "stand-alone", I can't take stuff that
requires core changes only for one staging driver.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel