| Message ID | CAOiWkA8NSv1VskZi4AnTgjt6p-NsRoTMzHqyAPwCsMKU-c9ujg@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
On 9 September 2016 at 19:42, Marty Faltesek <mfaltesek@google.com> wrote: > It's blocked by the code below which I tried to ifdef out, but then it > returns all 0's. > > diff --git a/drivers/net/wireless/ath/ath10k/debug.c > b/drivers/net/wireless/ath/ath10k/debug.c > index 8b01e3e..bb8b7ec 100644 > --- a/drivers/net/wireless/ath/ath10k/debug.c > +++ b/drivers/net/wireless/ath/ath10k/debug.c > @@ -1433,12 +1433,13 @@ static int ath10k_debug_cal_data_open(struct > inode *inode, struct file *file) > int ret; > > mutex_lock(&ar->conf_mutex); > - > +#if 0 > if (ar->state != ATH10K_STATE_ON && > ar->state != ATH10K_STATE_UTF) { > ret = -ENETDOWN; > goto err; > } > +#endif This won't work. The driver needs to go through ath10k_start(), i.e. firmware must be loaded. Cal data is cooked as part of that. You could get away with just `ifconfig wlan0 up`. You don't need to connect or anything. I guess the driver *could* cache the end resulting cal data when the device is probed and re-use it in subsequent firmware boot-up attempts (instead of re-computing it) making it available when the device is stopped as well. Any volunteers to try *and* test if it doesn't break anything? :) > On Fri, Sep 9, 2016 at 1:39 PM, Adrian Chadd <adrian@freebsd.org> wrote: >> Hi, >> >> It's just in OTP? You should be able to read the OTP data without >> needing a STA/AP up? I would argue with the "just OTP" at least from the driver-device protocol point of view. Michał
On Mon, Sep 12, 2016 at 4:50 AM, Michal Kazior <michal.kazior@tieto.com> wrote: > On 9 September 2016 at 19:42, Marty Faltesek <mfaltesek@google.com> wrote: >> It's blocked by the code below which I tried to ifdef out, but then it >> returns all 0's. >> >> diff --git a/drivers/net/wireless/ath/ath10k/debug.c >> b/drivers/net/wireless/ath/ath10k/debug.c >> index 8b01e3e..bb8b7ec 100644 >> --- a/drivers/net/wireless/ath/ath10k/debug.c >> +++ b/drivers/net/wireless/ath/ath10k/debug.c >> @@ -1433,12 +1433,13 @@ static int ath10k_debug_cal_data_open(struct >> inode *inode, struct file *file) >> int ret; >> >> mutex_lock(&ar->conf_mutex); >> - >> +#if 0 >> if (ar->state != ATH10K_STATE_ON && >> ar->state != ATH10K_STATE_UTF) { >> ret = -ENETDOWN; >> goto err; >> } >> +#endif > > This won't work. The driver needs to go through ath10k_start(), i.e. > firmware must be loaded. Cal data is cooked as part of that. > > You could get away with just `ifconfig wlan0 up`. You don't need to > connect or anything. This does not work: hexdump: ./kernel/debug/ieee80211/phy1/ath10k/cal_data: Network is down But it works after starting the AP. Are you sure about what you said? > > I guess the driver *could* cache the end resulting cal data when the > device is probed and re-use it in subsequent firmware boot-up attempts > (instead of re-computing it) making it available when the device is > stopped as well. Any volunteers to try *and* test if it doesn't break > anything? :) This might break what we are trying to accomplish, which is read the calibration data and search for mis-calibrated hardware, and if found, apply a fix. > > >> On Fri, Sep 9, 2016 at 1:39 PM, Adrian Chadd <adrian@freebsd.org> wrote: >>> Hi, >>> >>> It's just in OTP? You should be able to read the OTP data without >>> needing a STA/AP up? > > I would argue with the "just OTP" at least from the driver-device > protocol point of view. > > > Michał
On Mon, Sep 12, 2016 at 4:50 AM, Michal Kazior <michal.kazior@tieto.com> wrote: > On 9 September 2016 at 19:42, Marty Faltesek <mfaltesek@google.com> wrote: >> It's blocked by the code below which I tried to ifdef out, but then it >> returns all 0's. >> >> diff --git a/drivers/net/wireless/ath/ath10k/debug.c >> b/drivers/net/wireless/ath/ath10k/debug.c >> index 8b01e3e..bb8b7ec 100644 >> --- a/drivers/net/wireless/ath/ath10k/debug.c >> +++ b/drivers/net/wireless/ath/ath10k/debug.c >> @@ -1433,12 +1433,13 @@ static int ath10k_debug_cal_data_open(struct >> inode *inode, struct file *file) >> int ret; >> >> mutex_lock(&ar->conf_mutex); >> - >> +#if 0 >> if (ar->state != ATH10K_STATE_ON && >> ar->state != ATH10K_STATE_UTF) { >> ret = -ENETDOWN; >> goto err; >> } >> +#endif > > This won't work. The driver needs to go through ath10k_start(), i.e. > firmware must be loaded. Cal data is cooked as part of that. > > You could get away with just `ifconfig wlan0 up`. You don't need to > connect or anything. > > I guess the driver *could* cache the end resulting cal data when the > device is probed and re-use it in subsequent firmware boot-up attempts > (instead of re-computing it) making it available when the device is > stopped as well. Any volunteers to try *and* test if it doesn't break > anything? :) > I see what you mean now. This might actually work for us. I'll give it a shot and see. thanks, Marty
Marty Faltesek <mfaltesek@google.com> writes: > On Mon, Sep 12, 2016 at 4:50 AM, Michal Kazior <michal.kazior@tieto.com> wrote: >> On 9 September 2016 at 19:42, Marty Faltesek <mfaltesek@google.com> wrote: >>> It's blocked by the code below which I tried to ifdef out, but then it >>> returns all 0's. >>> >>> diff --git a/drivers/net/wireless/ath/ath10k/debug.c >>> b/drivers/net/wireless/ath/ath10k/debug.c >>> index 8b01e3e..bb8b7ec 100644 >>> --- a/drivers/net/wireless/ath/ath10k/debug.c >>> +++ b/drivers/net/wireless/ath/ath10k/debug.c >>> @@ -1433,12 +1433,13 @@ static int ath10k_debug_cal_data_open(struct >>> inode *inode, struct file *file) >>> int ret; >>> >>> mutex_lock(&ar->conf_mutex); >>> - >>> +#if 0 >>> if (ar->state != ATH10K_STATE_ON && >>> ar->state != ATH10K_STATE_UTF) { >>> ret = -ENETDOWN; >>> goto err; >>> } >>> +#endif >> >> This won't work. The driver needs to go through ath10k_start(), i.e. >> firmware must be loaded. Cal data is cooked as part of that. >> >> You could get away with just `ifconfig wlan0 up`. You don't need to >> connect or anything. > > This does not work: > > hexdump: ./kernel/debug/ieee80211/phy1/ath10k/cal_data: Network is down > > But it works after starting the AP. Are you sure about what you said? It should work: # ip link show wlan0 5: wlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 link/ether 00:03:7f:48:d6:05 brd ff:ff:ff:ff:ff:ff # ip link set wlan0 up # hexdump /sys/kernel/debug/ieee80211/phy0/ath10k/cal_data # ip link set wlan0 down # hexdump /sys/kernel/debug/ieee80211/phy0/ath10k/cal_data hexdump: /sys/kernel/debug/ieee80211/phy0/ath10k/cal_data: Network is down # What's odd is that I got a file with zero bytes, but no time to investigate it now.
Hey Kalle OK this does work for me after all. Thanks. I wrote a patch to cache cal_data only while the core is off. I don't need it now, but wondering if there is any benefit to submitting it? thanks, Marty On Tue, Sep 13, 2016 at 6:46 AM, Valo, Kalle <kvalo@qca.qualcomm.com> wrote: > Marty Faltesek <mfaltesek@google.com> writes: > >> On Mon, Sep 12, 2016 at 4:50 AM, Michal Kazior <michal.kazior@tieto.com> wrote: >>> On 9 September 2016 at 19:42, Marty Faltesek <mfaltesek@google.com> wrote: >>>> It's blocked by the code below which I tried to ifdef out, but then it >>>> returns all 0's. >>>> >>>> diff --git a/drivers/net/wireless/ath/ath10k/debug.c >>>> b/drivers/net/wireless/ath/ath10k/debug.c >>>> index 8b01e3e..bb8b7ec 100644 >>>> --- a/drivers/net/wireless/ath/ath10k/debug.c >>>> +++ b/drivers/net/wireless/ath/ath10k/debug.c >>>> @@ -1433,12 +1433,13 @@ static int ath10k_debug_cal_data_open(struct >>>> inode *inode, struct file *file) >>>> int ret; >>>> >>>> mutex_lock(&ar->conf_mutex); >>>> - >>>> +#if 0 >>>> if (ar->state != ATH10K_STATE_ON && >>>> ar->state != ATH10K_STATE_UTF) { >>>> ret = -ENETDOWN; >>>> goto err; >>>> } >>>> +#endif >>> >>> This won't work. The driver needs to go through ath10k_start(), i.e. >>> firmware must be loaded. Cal data is cooked as part of that. >>> >>> You could get away with just `ifconfig wlan0 up`. You don't need to >>> connect or anything. >> >> This does not work: >> >> hexdump: ./kernel/debug/ieee80211/phy1/ath10k/cal_data: Network is down >> >> But it works after starting the AP. Are you sure about what you said? > > It should work: > > # ip link show wlan0 > 5: wlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 > link/ether 00:03:7f:48:d6:05 brd ff:ff:ff:ff:ff:ff > # ip link set wlan0 up > # hexdump /sys/kernel/debug/ieee80211/phy0/ath10k/cal_data > # ip link set wlan0 down > # hexdump /sys/kernel/debug/ieee80211/phy0/ath10k/cal_data > hexdump: /sys/kernel/debug/ieee80211/phy0/ath10k/cal_data: Network is down > # > > What's odd is that I got a file with zero bytes, but no time to > investigate it now. > > -- > Kalle Valo
Yes! :) -a On 13 September 2016 at 13:49, Marty Faltesek <mfaltesek@google.com> wrote: > Hey Kalle > > OK this does work for me after all. Thanks. > > I wrote a patch to cache cal_data only while the core is off. I don't > need it now, but wondering if there is any benefit > to submitting it? > > thanks, > > Marty > > > > > > On Tue, Sep 13, 2016 at 6:46 AM, Valo, Kalle <kvalo@qca.qualcomm.com> wrote: >> Marty Faltesek <mfaltesek@google.com> writes: >> >>> On Mon, Sep 12, 2016 at 4:50 AM, Michal Kazior <michal.kazior@tieto.com> wrote: >>>> On 9 September 2016 at 19:42, Marty Faltesek <mfaltesek@google.com> wrote: >>>>> It's blocked by the code below which I tried to ifdef out, but then it >>>>> returns all 0's. >>>>> >>>>> diff --git a/drivers/net/wireless/ath/ath10k/debug.c >>>>> b/drivers/net/wireless/ath/ath10k/debug.c >>>>> index 8b01e3e..bb8b7ec 100644 >>>>> --- a/drivers/net/wireless/ath/ath10k/debug.c >>>>> +++ b/drivers/net/wireless/ath/ath10k/debug.c >>>>> @@ -1433,12 +1433,13 @@ static int ath10k_debug_cal_data_open(struct >>>>> inode *inode, struct file *file) >>>>> int ret; >>>>> >>>>> mutex_lock(&ar->conf_mutex); >>>>> - >>>>> +#if 0 >>>>> if (ar->state != ATH10K_STATE_ON && >>>>> ar->state != ATH10K_STATE_UTF) { >>>>> ret = -ENETDOWN; >>>>> goto err; >>>>> } >>>>> +#endif >>>> >>>> This won't work. The driver needs to go through ath10k_start(), i.e. >>>> firmware must be loaded. Cal data is cooked as part of that. >>>> >>>> You could get away with just `ifconfig wlan0 up`. You don't need to >>>> connect or anything. >>> >>> This does not work: >>> >>> hexdump: ./kernel/debug/ieee80211/phy1/ath10k/cal_data: Network is down >>> >>> But it works after starting the AP. Are you sure about what you said? >> >> It should work: >> >> # ip link show wlan0 >> 5: wlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 >> link/ether 00:03:7f:48:d6:05 brd ff:ff:ff:ff:ff:ff >> # ip link set wlan0 up >> # hexdump /sys/kernel/debug/ieee80211/phy0/ath10k/cal_data >> # ip link set wlan0 down >> # hexdump /sys/kernel/debug/ieee80211/phy0/ath10k/cal_data >> hexdump: /sys/kernel/debug/ieee80211/phy0/ath10k/cal_data: Network is down >> # >> >> What's odd is that I got a file with zero bytes, but no time to >> investigate it now. >> >> -- >> Kalle Valo
Marty Faltesek <mfaltesek@google.com> writes: > OK this does work for me after all. Thanks. Great. > I wrote a patch to cache cal_data only while the core is off. I don't > need it now, but wondering if there is any benefit > to submitting it? I guess Adrian already answered that :) So please do submit and let's take a look at it.
I sent out the patch. I ran into another related issue. Let me explain what we are trying to do: We want to examine cal_data for a possibly mis-calibrated OTP, and if so, patch it and store the result in /tmp (creating a symlink from /ilb/firmware/ath10k/cal-pci-0001:01:00.0.bin to /tmp). But ath10k_fetch_cal_file() is only read once at load time, so I've modified the code to call this function from ath10k_download_cal_file() so that the driver catches the modified cal_data. This seems to work based on the read back of the patched cal_data, but is it the right approach, or is there a better or cleaner way? On Wed, Sep 14, 2016 at 1:01 AM, Valo, Kalle <kvalo@qca.qualcomm.com> wrote: > Marty Faltesek <mfaltesek@google.com> writes: > >> OK this does work for me after all. Thanks. > > Great. > >> I wrote a patch to cache cal_data only while the core is off. I don't >> need it now, but wondering if there is any benefit >> to submitting it? > > I guess Adrian already answered that :) So please do submit and let's > take a look at it. > > -- > Kalle Valo
On 15 September 2016 at 01:54, Marty Faltesek <mfaltesek@google.com> wrote: > I sent out the patch. > > I ran into another related issue. Let me explain what we are trying to do: > > We want to examine cal_data for a possibly mis-calibrated OTP, and if > so, patch it and store > the result in /tmp (creating a symlink from > /ilb/firmware/ath10k/cal-pci-0001:01:00.0.bin to /tmp). > > But ath10k_fetch_cal_file() is only read once at load time, so I've > modified the code to call this function from > ath10k_download_cal_file() > so that the driver catches the modified cal_data. This seems to work > based on the read back of the patched cal_data, but is it the right > approach, or is there a better or cleaner way? The reason why firmware files (including cal, et al) in memory is to guarantee firmware reloads result in the same set of hw capabilities which can be advertised during mac80211 register time. Changing them in runtime is not really clean. I don't really have a clean answer for you. Do you really need to keep given driver instance running without interruption? At what point do you verify caldata? If it's before upping interfaces you could either unload/load the driver or unbind/bind the device itself to force it to read firmware files anew. Michał
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c index 8b01e3e..bb8b7ec 100644 --- a/drivers/net/wireless/ath/ath10k/debug.c +++ b/drivers/net/wireless/ath/ath10k/debug.c @@ -1433,12 +1433,13 @@ static int ath10k_debug_cal_data_open(struct inode *inode, struct file *file) int ret; mutex_lock(&ar->conf_mutex); - +#if 0 if (ar->state != ATH10K_STATE_ON && ar->state != ATH10K_STATE_UTF) { ret = -ENETDOWN; goto err; } +#endif On Fri, Sep 9, 2016 at 1:39 PM, Adrian Chadd <adrian@freebsd.org> wrote: