| Message ID | 1479141222-8493-3-git-send-email-erik.stromdahl@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
On 14 November 2016 at 17:33, Erik Stromdahl <erik.stromdahl@gmail.com> wrote: > The RX trailer parsing is now capable of parsing lookahead reports. > This is needed by SDIO/mbox. It'd be useful to know what exactly lookahead will be used for. What payload does it carry. > Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com> > --- > drivers/net/wireless/ath/ath10k/htc.c | 91 ++++++++++++++++++++++++++++++++- > drivers/net/wireless/ath/ath10k/htc.h | 30 +++++++++-- > 2 files changed, 116 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c > index 26b1e15..e3f7bf4 100644 > --- a/drivers/net/wireless/ath/ath10k/htc.c > +++ b/drivers/net/wireless/ath/ath10k/htc.c > @@ -228,10 +228,74 @@ void ath10k_htc_tx_completion_handler(struct ath10k *ar, struct sk_buff *skb) > spin_unlock_bh(&htc->tx_lock); > } > > +static int > +ath10k_htc_process_lookahead(struct ath10k_htc *htc, > + const struct ath10k_htc_lookahead_report *report, > + int len, > + enum ath10k_htc_ep_id eid, > + u32 *next_lk_ahds, next_lk_ahds should be u8 or void. From what I understand by glancing through the code it is an agnostic buffer that carries payload which is later interpreted either as eid or htc header, right? void is probably most suitable in this case for passing around and u8 for inline-based storage. I'd also avoid silly abbreviations. Probably "lookahead" alone is enough. > + int *next_lk_ahds_len) > +{ > + struct ath10k *ar = htc->ar; > + > + if (report->pre_valid != ((~report->post_valid) & 0xFF)) > + /* Invalid lookahead flags are actually transmitted by > + * the target in the HTC control message. > + * Since this will happen at every boot we silently ignore > + * the lookahead in this case > + */ I'd put this comment before the if(). > + return 0; > + > + if (next_lk_ahds && next_lk_ahds_len) { > + ath10k_dbg(ar, ATH10K_DBG_HTC, > + "htc rx lk_ahd found pre_valid 0x%x post_valid 0x%x\n", > + report->pre_valid, report->post_valid); > + > + /* look ahead bytes are valid, copy them over */ > + memcpy((u8 *)&next_lk_ahds[0], report->lk_ahd, 4); > + > + *next_lk_ahds_len = 1; > + } > + > + return 0; > +} > + > +static int > +ath10k_htc_process_lookahead_bundle(struct ath10k_htc *htc, > + const struct ath10k_htc_lookahead_report_bundle *report, > + int len, > + enum ath10k_htc_ep_id eid, > + u32 *next_lk_ahds, Ditto. void. > + int *next_lk_ahds_len) > +{ > + int bundle_cnt = len / sizeof(*report); > + > + if (!bundle_cnt || (bundle_cnt > HTC_HOST_MAX_MSG_PER_BUNDLE)) { > + WARN_ON(1); This should be ath10k_warn() instead. This isn't internal driver flow assertion. It is possible firmware bug or revision misalignment instead. > + return -EINVAL; > + } > + > + if (next_lk_ahds && next_lk_ahds_len) { > + int i; > + > + for (i = 0; i < bundle_cnt; i++) { > + memcpy((u8 *)&next_lk_ahds[i], report->lk_ahd, > + sizeof(u32)); You'll need to re-adjust the &x[i] to maintain proper offset with void pointer. Michał
On 11/15/2016 10:57 AM, Michal Kazior wrote: > On 14 November 2016 at 17:33, Erik Stromdahl <erik.stromdahl@gmail.com> wrote: >> The RX trailer parsing is now capable of parsing lookahead reports. >> This is needed by SDIO/mbox. > > It'd be useful to know what exactly lookahead will be used for. What > payload does it carry. > It carries the 4 first bytes of the next RX message, i.e. the first 4 bytes of an HTC header. It is used by the sdio interrupt handler to know if the next packet is a part of an RX bundle, which endpoint it belongs to and how long it is (so we know how many bytes to allocate). > >> Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com> >> --- >> drivers/net/wireless/ath/ath10k/htc.c | 91 ++++++++++++++++++++++++++++++++- >> drivers/net/wireless/ath/ath10k/htc.h | 30 +++++++++-- >> 2 files changed, 116 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c >> index 26b1e15..e3f7bf4 100644 >> --- a/drivers/net/wireless/ath/ath10k/htc.c >> +++ b/drivers/net/wireless/ath/ath10k/htc.c >> @@ -228,10 +228,74 @@ void ath10k_htc_tx_completion_handler(struct ath10k *ar, struct sk_buff *skb) >> spin_unlock_bh(&htc->tx_lock); >> } >> >> +static int >> +ath10k_htc_process_lookahead(struct ath10k_htc *htc, >> + const struct ath10k_htc_lookahead_report *report, >> + int len, >> + enum ath10k_htc_ep_id eid, >> + u32 *next_lk_ahds, > > next_lk_ahds should be u8 or void. From what I understand by glancing > through the code it is an agnostic buffer that carries payload which > is later interpreted either as eid or htc header, right? void is > probably most suitable in this case for passing around and u8 for > inline-based storage. > Sounds reasonable, I'll change to void pointer. > I'd also avoid silly abbreviations. Probably "lookahead" alone is enough. > Ok, this abbreviation was actually taken from the ath6kl code. I think the intention was to reduce line lengths. >> + int *next_lk_ahds_len) >> +{ >> + struct ath10k *ar = htc->ar; >> + >> + if (report->pre_valid != ((~report->post_valid) & 0xFF)) >> + /* Invalid lookahead flags are actually transmitted by >> + * the target in the HTC control message. >> + * Since this will happen at every boot we silently ignore >> + * the lookahead in this case >> + */ > > I'd put this comment before the if(). Ok > > >> + return 0; >> + >> + if (next_lk_ahds && next_lk_ahds_len) { >> + ath10k_dbg(ar, ATH10K_DBG_HTC, >> + "htc rx lk_ahd found pre_valid 0x%x post_valid 0x%x\n", >> + report->pre_valid, report->post_valid); >> + >> + /* look ahead bytes are valid, copy them over */ >> + memcpy((u8 *)&next_lk_ahds[0], report->lk_ahd, 4); >> + >> + *next_lk_ahds_len = 1; >> + } >> + >> + return 0; >> +} >> + >> +static int >> +ath10k_htc_process_lookahead_bundle(struct ath10k_htc *htc, >> + const struct ath10k_htc_lookahead_report_bundle *report, >> + int len, >> + enum ath10k_htc_ep_id eid, >> + u32 *next_lk_ahds, > > Ditto. void. > > >> + int *next_lk_ahds_len) >> +{ >> + int bundle_cnt = len / sizeof(*report); >> + >> + if (!bundle_cnt || (bundle_cnt > HTC_HOST_MAX_MSG_PER_BUNDLE)) { >> + WARN_ON(1); > > This should be ath10k_warn() instead. This isn't internal driver flow > assertion. It is possible firmware bug or revision misalignment > instead. > Ok > >> + return -EINVAL; >> + } >> + >> + if (next_lk_ahds && next_lk_ahds_len) { >> + int i; >> + >> + for (i = 0; i < bundle_cnt; i++) { >> + memcpy((u8 *)&next_lk_ahds[i], report->lk_ahd, >> + sizeof(u32)); > > You'll need to re-adjust the &x[i] to maintain proper offset with void pointer. > > > Michał >
Erik Stromdahl <erik.stromdahl@gmail.com> writes: > On 11/15/2016 10:57 AM, Michal Kazior wrote: >> On 14 November 2016 at 17:33, Erik Stromdahl <erik.stromdahl@gmail.com> wrote: >>> The RX trailer parsing is now capable of parsing lookahead reports. >>> This is needed by SDIO/mbox. >> >> It'd be useful to know what exactly lookahead will be used for. What >> payload does it carry. >> > It carries the 4 first bytes of the next RX message, i.e. the first 4 > bytes of an HTC header. > > It is used by the sdio interrupt handler to know if the next packet is a > part of an RX bundle, which endpoint it belongs to and how long it is > (so we know how many bytes to allocate). Please add that to the commit log, it's useful information. Or maybe a code comment would be better? >>> +static int >>> +ath10k_htc_process_lookahead(struct ath10k_htc *htc, >>> + const struct ath10k_htc_lookahead_report *report, >>> + int len, >>> + enum ath10k_htc_ep_id eid, >>> + u32 *next_lk_ahds, >> >> next_lk_ahds should be u8 or void. From what I understand by glancing >> through the code it is an agnostic buffer that carries payload which >> is later interpreted either as eid or htc header, right? void is >> probably most suitable in this case for passing around and u8 for >> inline-based storage. >> > Sounds reasonable, I'll change to void pointer. > >> I'd also avoid silly abbreviations. Probably "lookahead" alone is enough. >> > Ok, this abbreviation was actually taken from the ath6kl code. I think > the intention was to reduce line lengths. Most likely that comes from the staging ath6kl driver which again is a fork of the Atheros internal driver. I agree with Michal, better to avoid silly abbreviations as much as possible. Readability is most important.
diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c index 26b1e15..e3f7bf4 100644 --- a/drivers/net/wireless/ath/ath10k/htc.c +++ b/drivers/net/wireless/ath/ath10k/htc.c @@ -228,10 +228,74 @@ void ath10k_htc_tx_completion_handler(struct ath10k *ar, struct sk_buff *skb) spin_unlock_bh(&htc->tx_lock); } +static int +ath10k_htc_process_lookahead(struct ath10k_htc *htc, + const struct ath10k_htc_lookahead_report *report, + int len, + enum ath10k_htc_ep_id eid, + u32 *next_lk_ahds, + int *next_lk_ahds_len) +{ + struct ath10k *ar = htc->ar; + + if (report->pre_valid != ((~report->post_valid) & 0xFF)) + /* Invalid lookahead flags are actually transmitted by + * the target in the HTC control message. + * Since this will happen at every boot we silently ignore + * the lookahead in this case + */ + return 0; + + if (next_lk_ahds && next_lk_ahds_len) { + ath10k_dbg(ar, ATH10K_DBG_HTC, + "htc rx lk_ahd found pre_valid 0x%x post_valid 0x%x\n", + report->pre_valid, report->post_valid); + + /* look ahead bytes are valid, copy them over */ + memcpy((u8 *)&next_lk_ahds[0], report->lk_ahd, 4); + + *next_lk_ahds_len = 1; + } + + return 0; +} + +static int +ath10k_htc_process_lookahead_bundle(struct ath10k_htc *htc, + const struct ath10k_htc_lookahead_report_bundle *report, + int len, + enum ath10k_htc_ep_id eid, + u32 *next_lk_ahds, + int *next_lk_ahds_len) +{ + int bundle_cnt = len / sizeof(*report); + + if (!bundle_cnt || (bundle_cnt > HTC_HOST_MAX_MSG_PER_BUNDLE)) { + WARN_ON(1); + return -EINVAL; + } + + if (next_lk_ahds && next_lk_ahds_len) { + int i; + + for (i = 0; i < bundle_cnt; i++) { + memcpy((u8 *)&next_lk_ahds[i], report->lk_ahd, + sizeof(u32)); + report++; + } + + *next_lk_ahds_len = bundle_cnt; + } + + return 0; +} + int ath10k_htc_process_trailer(struct ath10k_htc *htc, u8 *buffer, int length, - enum ath10k_htc_ep_id src_eid) + enum ath10k_htc_ep_id src_eid, + u32 *next_lk_ahds, + int *next_lk_ahds_len) { struct ath10k *ar = htc->ar; int status = 0; @@ -272,6 +336,28 @@ int ath10k_htc_process_trailer(struct ath10k_htc *htc, record->hdr.len, src_eid); break; + case ATH10K_HTC_RECORD_LOOKAHEAD: + len = sizeof(struct ath10k_htc_lookahead_report); + if (record->hdr.len < len) { + ath10k_warn(ar, "Lookahead report too long\n"); + status = -EINVAL; + break; + } + status = ath10k_htc_process_lookahead(htc, + record->lkahd_report, + record->hdr.len, + src_eid, + next_lk_ahds, + next_lk_ahds_len); + break; + case ATH10K_HTC_RECORD_LOOKAHEAD_BUNDLE: + status = ath10k_htc_process_lookahead_bundle(htc, + record->lkahd_bundle, + record->hdr.len, + src_eid, + next_lk_ahds, + next_lk_ahds_len); + break; default: ath10k_warn(ar, "Unhandled record: id:%d length:%d\n", record->hdr.id, record->hdr.len); @@ -359,7 +445,8 @@ void ath10k_htc_rx_completion_handler(struct ath10k *ar, struct sk_buff *skb) trailer += payload_len; trailer -= trailer_len; status = ath10k_htc_process_trailer(htc, trailer, - trailer_len, hdr->eid); + trailer_len, hdr->eid, + NULL, NULL); if (status) goto out; diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h index 858e19f..589800a 100644 --- a/drivers/net/wireless/ath/ath10k/htc.h +++ b/drivers/net/wireless/ath/ath10k/htc.h @@ -50,6 +50,8 @@ * 4-byte aligned. */ +#define HTC_HOST_MAX_MSG_PER_BUNDLE 8 + enum ath10k_htc_tx_flags { ATH10K_HTC_FLAG_NEED_CREDIT_UPDATE = 0x01, ATH10K_HTC_FLAG_SEND_BUNDLE = 0x02 @@ -174,8 +176,10 @@ struct ath10k_htc_msg { } __packed __aligned(4); enum ath10k_ath10k_htc_record_id { - ATH10K_HTC_RECORD_NULL = 0, - ATH10K_HTC_RECORD_CREDITS = 1 + ATH10K_HTC_RECORD_NULL = 0, + ATH10K_HTC_RECORD_CREDITS = 1, + ATH10K_HTC_RECORD_LOOKAHEAD = 2, + ATH10K_HTC_RECORD_LOOKAHEAD_BUNDLE = 3, }; struct ath10k_ath10k_htc_record_hdr { @@ -192,10 +196,28 @@ struct ath10k_htc_credit_report { u8 pad1; } __packed; +struct ath10k_htc_lookahead_report { + u8 pre_valid; + u8 pad0; + u8 pad1; + u8 pad2; + u8 lk_ahd[4]; + u8 post_valid; + u8 pad3; + u8 pad4; + u8 pad5; +} __packed; + +struct ath10k_htc_lookahead_report_bundle { + u8 lk_ahd[4]; +} __packed; + struct ath10k_htc_record { struct ath10k_ath10k_htc_record_hdr hdr; union { struct ath10k_htc_credit_report credit_report[0]; + struct ath10k_htc_lookahead_report lkahd_report[0]; + struct ath10k_htc_lookahead_report_bundle lkahd_bundle[0]; u8 pauload[0]; }; } __packed __aligned(4); @@ -359,6 +381,8 @@ void ath10k_htc_notify_tx_completion(struct ath10k_htc_ep *ep, int ath10k_htc_process_trailer(struct ath10k_htc *htc, u8 *buffer, int length, - enum ath10k_htc_ep_id src_eid); + enum ath10k_htc_ep_id src_eid, + u32 *next_lk_ahds, + int *next_lk_ahds_len); #endif
The RX trailer parsing is now capable of parsing lookahead reports. This is needed by SDIO/mbox. Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com> --- drivers/net/wireless/ath/ath10k/htc.c | 91 ++++++++++++++++++++++++++++++++- drivers/net/wireless/ath/ath10k/htc.h | 30 +++++++++-- 2 files changed, 116 insertions(+), 5 deletions(-)