| Message ID | 1513270393-919-7-git-send-email-qca_merez@qca.qualcomm.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Kalle Valo |
| Headers | show |
Maya Erez <qca_merez@qca.qualcomm.com> wrote: > Add module parameter for configuring the headroom size > in the skb allocation. > > Signed-off-by: Lazar Alexei <qca_ailizaro@qca.qualcomm.com> > Signed-off-by: Maya Erez <qca_merez@qca.qualcomm.com> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> Why? https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#commit_log_does_not_answer_why And I'm a bit skeptic about this, controlling headroom via a module parameter is not really making any sense to me.
On 2018-01-09 10:07, Kalle Valo wrote: > Maya Erez <qca_merez@qca.qualcomm.com> wrote: > >> Add module parameter for configuring the headroom size >> in the skb allocation. >> >> Signed-off-by: Lazar Alexei <qca_ailizaro@qca.qualcomm.com> >> Signed-off-by: Maya Erez <qca_merez@qca.qualcomm.com> >> Signed-off-by: Kalle Valo <kvalo@codeaurora.org> > > Why? Some platforms have specific requirements on packet alignment. I will upload an update of the commit text in the next set of 11ad patches. > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#commit_log_does_not_answer_why > > And I'm a bit skeptic about this, controlling headroom via a module > parameter > is not really making any sense to me.
Hi Maya, On Tue, Jan 16, 2018 at 12:18 AM, <merez@codeaurora.org> wrote: > On 2018-01-09 10:07, Kalle Valo wrote: >> >> Maya Erez <qca_merez@qca.qualcomm.com> wrote: >> >>> Add module parameter for configuring the headroom size >>> in the skb allocation. >>> >>> Signed-off-by: Lazar Alexei <qca_ailizaro@qca.qualcomm.com> >>> Signed-off-by: Maya Erez <qca_merez@qca.qualcomm.com> >>> Signed-off-by: Kalle Valo <kvalo@codeaurora.org> >> >> >> Why? > > > Some platforms have specific requirements on packet alignment. > I will upload an update of the commit text in the next set of 11ad patches. Is this something the platform can tell you or something you can store in an array by platform name? (and use the maximum size if we don't know) Alternatively, would it waste too much RAM to just set this to the maximum size and have done? I feel that allowing the user to set it will be problematic. Thanks,
Julian Calaby <julian.calaby@gmail.com> writes: > On Tue, Jan 16, 2018 at 12:18 AM, <merez@codeaurora.org> wrote: >> On 2018-01-09 10:07, Kalle Valo wrote: >>> >>> Maya Erez <qca_merez@qca.qualcomm.com> wrote: >>> >>>> Add module parameter for configuring the headroom size >>>> in the skb allocation. >>>> >>>> Signed-off-by: Lazar Alexei <qca_ailizaro@qca.qualcomm.com> >>>> Signed-off-by: Maya Erez <qca_merez@qca.qualcomm.com> >>>> Signed-off-by: Kalle Valo <kvalo@codeaurora.org> >>> >>> >>> Why? >> >> >> Some platforms have specific requirements on packet alignment. >> I will upload an update of the commit text in the next set of 11ad patches. > > Is this something the platform can tell you or something you can store > in an array by platform name? (and use the maximum size if we don't > know) > > Alternatively, would it waste too much RAM to just set this to the > maximum size and have done? > > I feel that allowing the user to set it will be problematic. I feel the same. I don't think this is something which should be controlled with a module parameter, instead the driver should choose it automatically.
diff --git a/drivers/net/wireless/ath/wil6210/txrx.c b/drivers/net/wireless/ath/wil6210/txrx.c index 62c04f0..89967ce 100644 --- a/drivers/net/wireless/ath/wil6210/txrx.c +++ b/drivers/net/wireless/ath/wil6210/txrx.c @@ -41,6 +41,32 @@ module_param(rx_large_buf, bool, 0444); MODULE_PARM_DESC(rx_large_buf, " allocate 8KB RX buffers, default - no"); +#define WIL6210_MAX_HEADROOM_SIZE (256) + +static ushort headroom_size; /* = 0; */ +static int headroom_size_set(const char *val, const struct kernel_param *kp) +{ + int ret; + + ret = param_set_uint(val, kp); + if (ret) + return ret; + + if (headroom_size > WIL6210_MAX_HEADROOM_SIZE) + return -EINVAL; + + return 0; +} + +static const struct kernel_param_ops headroom_ops = { + .set = headroom_size_set, + .get = param_get_ushort, +}; + +module_param_cb(headroom_size, &headroom_ops, &headroom_size, 0644); +MODULE_PARM_DESC(headroom_size, + " headroom size for rx skb allocation, default - 0"); + static inline uint wil_rx_snaplen(void) { return rx_align_2 ? 6 : 0; @@ -630,7 +656,7 @@ static int wil_rx_refill(struct wil6210_priv *wil, int count) u32 next_tail; int rc = 0; int headroom = ndev->type == ARPHRD_IEEE80211_RADIOTAP ? - WIL6210_RTAP_SIZE : 0; + WIL6210_RTAP_SIZE : headroom_size; for (; next_tail = wil_vring_next_tail(v), (next_tail != v->swhead) && (count-- > 0);