| Message ID | 1569268165-1639-5-git-send-email-pozega.tomislav@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Tomislav Požega <pozega.tomislav@gmail.com> writes: > Drop unneeded lines by moving skb data in both main and 10x WMI > pull service ready event operations. > > Signed-off-by: Tomislav Požega <pozega.tomislav@gmail.com> > --- > drivers/net/wireless/ath/ath10k/wmi.c | 6 ++---- > 1 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c > index 59d2d2a..8ab178c 100644 > --- a/drivers/net/wireless/ath/ath10k/wmi.c > +++ b/drivers/net/wireless/ath/ath10k/wmi.c > @@ -5345,13 +5345,12 @@ static int ath10k_wmi_alloc_host_mem(struct ath10k *ar, u32 req_id, > ath10k_wmi_main_op_pull_svc_rdy_ev(struct ath10k *ar, struct sk_buff *skb, > struct wmi_svc_rdy_ev_arg *arg) > { > - struct wmi_service_ready_event *ev; > + struct wmi_service_ready_event *ev = (void *)skb->data; > size_t i, n; > > if (skb->len < sizeof(*ev)) > return -EPROTO; > > - ev = (void *)skb->data; Actually I prefer the original style, so that we first check the data in skb is valid and only then assign the data to ev.
On 24/09/2019, Kalle Valo <kvalo@codeaurora.org> wrote: > Tomislav Požega <pozega.tomislav@gmail.com> writes: > >> Drop unneeded lines by moving skb data in both main and 10x WMI >> pull service ready event operations. >> >> Signed-off-by: Tomislav Požega <pozega.tomislav@gmail.com> >> --- >> drivers/net/wireless/ath/ath10k/wmi.c | 6 ++---- >> 1 files changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c >> b/drivers/net/wireless/ath/ath10k/wmi.c >> index 59d2d2a..8ab178c 100644 >> --- a/drivers/net/wireless/ath/ath10k/wmi.c >> +++ b/drivers/net/wireless/ath/ath10k/wmi.c >> @@ -5345,13 +5345,12 @@ static int ath10k_wmi_alloc_host_mem(struct ath10k >> *ar, u32 req_id, >> ath10k_wmi_main_op_pull_svc_rdy_ev(struct ath10k *ar, struct sk_buff >> *skb, >> struct wmi_svc_rdy_ev_arg *arg) >> { >> - struct wmi_service_ready_event *ev; >> + struct wmi_service_ready_event *ev = (void *)skb->data; >> size_t i, n; >> >> if (skb->len < sizeof(*ev)) >> return -EPROTO; >> >> - ev = (void *)skb->data; > > Actually I prefer the original style, so that we first check the data in > skb is valid and only then assign the data to ev. > > -- > Kalle Valo > It came to my mind that this might be the reason why the current driver did not give me warning about too short service ready event, but there was no warning about event length in either case. I even tested this with compat wireless from 2013. and there the situation was the opposite: in both cases there was warning about service ready length.
On 2019-09-24 00:49, Tom Psyborg wrote: > On 24/09/2019, Kalle Valo <kvalo@codeaurora.org> wrote: >> Tomislav Požega <pozega.tomislav@gmail.com> writes: >> Actually I prefer the original style, so that we first check the data >> in >> skb is valid and only then assign the data to ev. >> >> -- >> Kalle Valo >> > > It came to my mind that this might be the reason why the current > driver did not give me warning about too short service ready event, > but there was no warning about event length in either case. > I even tested this with compat wireless from 2013. and there the > situation was the opposite: in both cases there was warning about > service ready length. Hmmm, my understanding of the way the TLV WMI is supposed to work is that the individual data structures are extensible, and in the case where a data structure is received with a "short" length the recipient is supposed to zero-extend to the expected length, and then handle the "zeroed" field(s) appropriately. This is supposed to hold for both host=>firmware and firmware=>host. Since the wmi_service_ready_event has been extended over time this behavior is necessary in the case of a host built with the current format interfacing to a firmware built with an earlier version of the format. I'm not sure why ath10k isn't supporting this since the QTI "out of tree" driver (my area of focus) has that support.
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c index 59d2d2a..8ab178c 100644 --- a/drivers/net/wireless/ath/ath10k/wmi.c +++ b/drivers/net/wireless/ath/ath10k/wmi.c @@ -5345,13 +5345,12 @@ static int ath10k_wmi_alloc_host_mem(struct ath10k *ar, u32 req_id, ath10k_wmi_main_op_pull_svc_rdy_ev(struct ath10k *ar, struct sk_buff *skb, struct wmi_svc_rdy_ev_arg *arg) { - struct wmi_service_ready_event *ev; + struct wmi_service_ready_event *ev = (void *)skb->data; size_t i, n; if (skb->len < sizeof(*ev)) return -EPROTO; - ev = (void *)skb->data; skb_pull(skb, sizeof(*ev)); arg->min_tx_power = ev->hw_min_tx_power; arg->max_tx_power = ev->hw_max_tx_power; @@ -5387,13 +5386,12 @@ static int ath10k_wmi_alloc_host_mem(struct ath10k *ar, u32 req_id, ath10k_wmi_10x_op_pull_svc_rdy_ev(struct ath10k *ar, struct sk_buff *skb, struct wmi_svc_rdy_ev_arg *arg) { - struct wmi_10x_service_ready_event *ev; + struct wmi_10x_service_ready_event *ev = (void *)skb->data; int i, n; if (skb->len < sizeof(*ev)) return -EPROTO; - ev = (void *)skb->data; skb_pull(skb, sizeof(*ev)); arg->min_tx_power = ev->hw_min_tx_power; arg->max_tx_power = ev->hw_max_tx_power;
Drop unneeded lines by moving skb data in both main and 10x WMI pull service ready event operations. Signed-off-by: Tomislav Požega <pozega.tomislav@gmail.com> --- drivers/net/wireless/ath/ath10k/wmi.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)