* Re: [RFC v0 3/8] firmware: Factor out firmware load helpers
From: Dmitry Torokhov @ 2016-07-28 17:57 UTC (permalink / raw)
To: Daniel Wagner
Cc: Bastien Nocera, Bjorn Andersson, Greg Kroah-Hartman,
Johannes Berg, Kalle Valo, Ohad Ben-Cohen, linux-input,
linux-kselftest, linux-wireless, linux-kernel, Daniel Wagner
In-Reply-To: <1469692512-16863-4-git-send-email-wagi@monom.org>
On Thu, Jul 28, 2016 at 09:55:07AM +0200, Daniel Wagner wrote:
> +int __firmware_stat_wait(struct firmware_stat *fwst,
> + long timeout)
> +{
> + int err;
> + err = swait_event_interruptible_timeout(fwst->wq,
> + is_fw_sync_done(READ_ONCE(fwst->status)),
> + timeout);
> + if (err == 0 && fwst->status == FW_STATUS_ABORT)
> + return -ENOENT;
> +
> + return err;
> +}
> +EXPORT_SYMBOL(__firmware_stat_wait);
> +
> +void __firmware_stat_set(struct firmware_stat *fwst, unsigned long status)
> +{
> + WRITE_ONCE(fwst->status, status);
> + swake_up(&fwst->wq);
Do we need to notify everyone for FW_STATUS_LOADING status?
The driver users do not care for sure.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: staging: wilc1000: Reduce scope for a few variables in mac_ioctl()
From: SF Markus Elfring @ 2016-07-28 16:00 UTC (permalink / raw)
To: Lino Sanfilippo
Cc: LKML, kernel-janitors, linux-wireless, devel, Greg Kroah-Hartman,
Steve Caldwell
In-Reply-To: <trinity-308db848-f204-4ebe-9336-484347ffbf37-1469706334519@3capp-gmx-bs72>
> Which circumstances do "not any sense at all" imply?
Should the expression 'strlen("RSSI")' be passed for the parameter 'length' instead?
> I suggest to fix this since it is indeed a bug,
We can agree that this function implementation was broken for a while there.
> instead of doing "micro optimizations" - which is the last thing that code in the staging area
> needs (as IIRC you have already been told by others, including the staging maintainer).
The acceptance might grow a bit more for such software fine-tuning
(like refactoring around variable usage).
Regards,
Markus
^ permalink raw reply
* Re: [RFC v0 3/8] firmware: Factor out firmware load helpers
From: Dan Williams @ 2016-07-28 15:01 UTC (permalink / raw)
To: Daniel Wagner, Bastien Nocera, Bjorn Andersson, Dmitry Torokhov,
Greg Kroah-Hartman, Johannes Berg, Kalle Valo, Ohad Ben-Cohen
Cc: linux-input, linux-kselftest, linux-wireless, linux-kernel,
Daniel Wagner
In-Reply-To: <1469692512-16863-4-git-send-email-wagi@monom.org>
On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>
> Factor out the firmware loading synchronization code in order to
> allow
> drivers to reuse it. This also documents more clearly what is
> happening. This is especial useful the drivers which will be
> converted
> afterwards. Not everyone has to come with yet another way to handle
> it.
It's somewhat odd to me that the structure is "firmware_stat" but most
of the functions are "fw_loading_*". That seems inconsistent for a
structure that is (currently) only used by these functions.
I would personally do either:
a) "struct fw_load_status" and "fw_load_*()"
or
b) "struct firmware_load_stat" and "firmware_load_*()"
I'd also change the function names from "loading" -> "load", similar to
how userland has read(2), not reading(2).
Dan
> We use swait instead completion. complete() would only wake up one
> waiter, so complete_all() is used. complete_all() wakes max
> MAX_UINT/2
> waiters which is plenty in all cases. Though withh swait we just wait
> until the exptected status shows up and wake any waiter.
>
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> ---
> drivers/base/firmware_class.c | 112 +++++++++++++++++++-------------
> ----------
> include/linux/firmware.h | 71 ++++++++++++++++++++++++++
> 2 files changed, 122 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c
> b/drivers/base/firmware_class.c
> index 773fc30..bf1ca70 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -30,6 +30,7 @@
> #include <linux/syscore_ops.h>
> #include <linux/reboot.h>
> #include <linux/security.h>
> +#include <linux/swait.h>
>
> #include <generated/utsrelease.h>
>
> @@ -85,11 +86,36 @@ static inline bool fw_is_builtin_firmware(const
> struct firmware *fw)
> }
> #endif
>
> -enum {
> - FW_STATUS_LOADING,
> - FW_STATUS_DONE,
> - FW_STATUS_ABORT,
> -};
> +
> +#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE)
> && defined(MODULE))
> +
> +static inline bool is_fw_sync_done(unsigned long status)
> +{
> + return status == FW_STATUS_LOADED || status ==
> FW_STATUS_ABORT;
> +}
> +
> +int __firmware_stat_wait(struct firmware_stat *fwst,
> + long timeout)
> +{
> + int err;
> + err = swait_event_interruptible_timeout(fwst->wq,
> + is_fw_sync_done(READ_ONCE(fwst-
> >status)),
> + timeout);
> + if (err == 0 && fwst->status == FW_STATUS_ABORT)
> + return -ENOENT;
> +
> + return err;
> +}
> +EXPORT_SYMBOL(__firmware_stat_wait);
> +
> +void __firmware_stat_set(struct firmware_stat *fwst, unsigned long
> status)
> +{
> + WRITE_ONCE(fwst->status, status);
> + swake_up(&fwst->wq);
> +}
> +EXPORT_SYMBOL(__firmware_stat_set);
> +
> +#endif
>
> static int loading_timeout = 60; /* In seconds */
>
> @@ -138,9 +164,8 @@ struct firmware_cache {
> struct firmware_buf {
> struct kref ref;
> struct list_head list;
> - struct completion completion;
> + struct firmware_stat fwst;
> struct firmware_cache *fwc;
> - unsigned long status;
> void *data;
> size_t size;
> #ifdef CONFIG_FW_LOADER_USER_HELPER
> @@ -194,7 +219,7 @@ static struct firmware_buf
> *__allocate_fw_buf(const char *fw_name,
>
> kref_init(&buf->ref);
> buf->fwc = fwc;
> - init_completion(&buf->completion);
> + firmware_stat_init(&buf->fwst);
> #ifdef CONFIG_FW_LOADER_USER_HELPER
> INIT_LIST_HEAD(&buf->pending_list);
> #endif
> @@ -292,15 +317,6 @@ static const char * const fw_path[] = {
> module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
> MODULE_PARM_DESC(path, "customized firmware image search path with a
> higher priority than default path");
>
> -static void fw_finish_direct_load(struct device *device,
> - struct firmware_buf *buf)
> -{
> - mutex_lock(&fw_lock);
> - set_bit(FW_STATUS_DONE, &buf->status);
> - complete_all(&buf->completion);
> - mutex_unlock(&fw_lock);
> -}
> -
> static int fw_get_filesystem_firmware(struct device *device,
> struct firmware_buf *buf)
> {
> @@ -339,7 +355,7 @@ static int fw_get_filesystem_firmware(struct
> device *device,
> }
> dev_dbg(device, "direct-loading %s\n", buf->fw_id);
> buf->size = size;
> - fw_finish_direct_load(device, buf);
> + fw_loading_done(buf->fwst);
> break;
> }
> __putname(path);
> @@ -457,12 +473,11 @@ static void __fw_load_abort(struct firmware_buf
> *buf)
> * There is a small window in which user can write to
> 'loading'
> * between loading done and disappearance of 'loading'
> */
> - if (test_bit(FW_STATUS_DONE, &buf->status))
> + if (is_fw_loading_done(buf->fwst))
> return;
>
> list_del_init(&buf->pending_list);
> - set_bit(FW_STATUS_ABORT, &buf->status);
> - complete_all(&buf->completion);
> + fw_loading_abort(buf->fwst);
> }
>
> static void fw_load_abort(struct firmware_priv *fw_priv)
> @@ -475,9 +490,6 @@ static void fw_load_abort(struct firmware_priv
> *fw_priv)
> fw_priv->buf = NULL;
> }
>
> -#define is_fw_load_aborted(buf) \
> - test_bit(FW_STATUS_ABORT, &(buf)->status)
> -
> static LIST_HEAD(pending_fw_head);
>
> /* reboot notifier for avoid deadlock with usermode_lock */
> @@ -577,7 +589,7 @@ static ssize_t firmware_loading_show(struct
> device *dev,
>
> mutex_lock(&fw_lock);
> if (fw_priv->buf)
> - loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf-
> >status);
> + loading = is_fw_loading(fw_priv->buf->fwst);
> mutex_unlock(&fw_lock);
>
> return sprintf(buf, "%d\n", loading);
> @@ -632,23 +644,20 @@ static ssize_t firmware_loading_store(struct
> device *dev,
> switch (loading) {
> case 1:
> /* discarding any previous partial load */
> - if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) {
> + if (!is_fw_loading_done(fw_buf->fwst)) {
> for (i = 0; i < fw_buf->nr_pages; i++)
> __free_page(fw_buf->pages[i]);
> vfree(fw_buf->pages);
> fw_buf->pages = NULL;
> fw_buf->page_array_size = 0;
> fw_buf->nr_pages = 0;
> - set_bit(FW_STATUS_LOADING, &fw_buf->status);
> + fw_loading_start(fw_buf->fwst);
> }
> break;
> case 0:
> - if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
> + if (is_fw_loading(fw_buf->fwst)) {
> int rc;
>
> - set_bit(FW_STATUS_DONE, &fw_buf->status);
> - clear_bit(FW_STATUS_LOADING, &fw_buf-
> >status);
> -
> /*
> * Several loading requests may be pending
> on
> * one same firmware buf, so let all
> requests
> @@ -670,10 +679,11 @@ static ssize_t firmware_loading_store(struct
> device *dev,
> */
> list_del_init(&fw_buf->pending_list);
> if (rc) {
> - set_bit(FW_STATUS_ABORT, &fw_buf-
> >status);
> + fw_loading_abort(fw_buf->fwst);
> written = rc;
> + } else {
> + fw_loading_done(fw_buf->fwst);
> }
> - complete_all(&fw_buf->completion);
> break;
> }
> /* fallthrough */
> @@ -681,7 +691,7 @@ static ssize_t firmware_loading_store(struct
> device *dev,
> dev_err(dev, "%s: unexpected value (%d)\n",
> __func__, loading);
> /* fallthrough */
> case -1:
> - fw_load_abort(fw_priv);
> + fw_loading_abort(fw_buf->fwst);
> break;
> }
> out:
> @@ -702,7 +712,7 @@ static ssize_t firmware_data_read(struct file
> *filp, struct kobject *kobj,
>
> mutex_lock(&fw_lock);
> buf = fw_priv->buf;
> - if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
> + if (!buf || is_fw_loading_done(buf->fwst)) {
> ret_count = -ENODEV;
> goto out;
> }
> @@ -799,7 +809,7 @@ static ssize_t firmware_data_write(struct file
> *filp, struct kobject *kobj,
>
> mutex_lock(&fw_lock);
> buf = fw_priv->buf;
> - if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
> + if (!buf || is_fw_loading_done(buf->fwst)) {
> retval = -ENODEV;
> goto out;
> }
> @@ -917,8 +927,7 @@ static int _request_firmware_load(struct
> firmware_priv *fw_priv,
> timeout = MAX_JIFFY_OFFSET;
> }
>
> - retval = wait_for_completion_interruptible_timeout(&buf-
> >completion,
> - timeout);
> + retval = fw_loading_wait_timeout(buf->fwst, timeout);
> if (retval == -ERESTARTSYS || !retval) {
> mutex_lock(&fw_lock);
> fw_load_abort(fw_priv);
> @@ -927,7 +936,7 @@ static int _request_firmware_load(struct
> firmware_priv *fw_priv,
> retval = 0;
> }
>
> - if (is_fw_load_aborted(buf))
> + if (is_fw_loading_aborted(buf->fwst))
> retval = -EAGAIN;
> else if (!buf->data)
> retval = -ENOMEM;
> @@ -986,26 +995,6 @@ static inline void
> kill_requests_without_uevent(void) { }
>
> #endif /* CONFIG_FW_LOADER_USER_HELPER */
>
> -
> -/* wait until the shared firmware_buf becomes ready (or error) */
> -static int sync_cached_firmware_buf(struct firmware_buf *buf)
> -{
> - int ret = 0;
> -
> - mutex_lock(&fw_lock);
> - while (!test_bit(FW_STATUS_DONE, &buf->status)) {
> - if (is_fw_load_aborted(buf)) {
> - ret = -ENOENT;
> - break;
> - }
> - mutex_unlock(&fw_lock);
> - ret = wait_for_completion_interruptible(&buf-
> >completion);
> - mutex_lock(&fw_lock);
> - }
> - mutex_unlock(&fw_lock);
> - return ret;
> -}
> -
> /* prepare firmware and firmware_buf structs;
> * return 0 if a firmware is already assigned, 1 if need to load
> one,
> * or a negative error code
> @@ -1039,7 +1028,8 @@ _request_firmware_prepare(struct firmware
> **firmware_p, const char *name,
> firmware->priv = buf;
>
> if (ret > 0) {
> - ret = sync_cached_firmware_buf(buf);
> + ret = fw_loading_wait_timeout(buf->fwst,
> + firmware_loading_timeo
> ut());
> if (!ret) {
> fw_set_page_data(buf, firmware);
> return 0; /* assigned */
> @@ -1057,7 +1047,7 @@ static int assign_firmware_buf(struct firmware
> *fw, struct device *device,
> struct firmware_buf *buf = fw->priv;
>
> mutex_lock(&fw_lock);
> - if (!buf->size || is_fw_load_aborted(buf)) {
> + if (!buf->size || is_fw_loading_aborted(buf->fwst)) {
> mutex_unlock(&fw_lock);
> return -ENOENT;
> }
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index 5c41c5e..f584160 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -4,10 +4,17 @@
> #include <linux/types.h>
> #include <linux/compiler.h>
> #include <linux/gfp.h>
> +#include <linux/swait.h>
>
> #define FW_ACTION_NOHOTPLUG 0
> #define FW_ACTION_HOTPLUG 1
>
> +enum {
> + FW_STATUS_LOADING,
> + FW_STATUS_LOADED,
> + FW_STATUS_ABORT,
> +};
> +
> struct firmware {
> size_t size;
> const u8 *data;
> @@ -17,6 +24,11 @@ struct firmware {
> void *priv;
> };
>
> +struct firmware_stat {
> + unsigned long status;
> + struct swait_queue_head wq;
> +};
> +
> struct module;
> struct device;
>
> @@ -49,6 +61,36 @@ int request_firmware_direct(const struct firmware
> **fw, const char *name,
> struct device *device);
>
> void release_firmware(const struct firmware *fw);
> +
> +static inline void firmware_stat_init(struct firmware_stat *fwst)
> +{
> + init_swait_queue_head(&fwst->wq);
> +}
> +
> +static inline unsigned long __firmware_stat_get(struct firmware_stat
> *fwst)
> +{
> + return READ_ONCE(fwst->status);
> +}
> +void __firmware_stat_set(struct firmware_stat *fwst, unsigned long
> status);
> +int __firmware_stat_wait(struct firmware_stat *fwst, long timeout);
> +
> +#define fw_loading_start(fwst)
> \
> + __firmware_stat_set(&fwst, FW_STATUS_LOADING)
> +#define fw_loading_done(fwst)
> \
> + __firmware_stat_set(&fwst, FW_STATUS_LOADED)
> +#define fw_loading_abort(fwst)
> \
> + __firmware_stat_set(&fwst, FW_STATUS_ABORT)
> +#define fw_loading_wait(fwst)
> \
> + __firmware_stat_wait(&fwst, 0)
> +#define fw_loading_wait_timeout(fwst, timeout)
> \
> + __firmware_stat_wait(&fwst, timeout)
> +#define is_fw_loading(fwst) \
> + (__firmware_stat_get(&fwst) == FW_STATUS_LOADING)
> +#define is_fw_loading_done(fwst) \
> + (__firmware_stat_get(&fwst) == FW_STATUS_LOADED)
> +#define is_fw_loading_aborted(fwst) \
> + (__firmware_stat_get(&fwst) == FW_STATUS_ABORT)
> +
> #else
> static inline int request_firmware(const struct firmware **fw,
> const char *name,
> @@ -75,5 +117,34 @@ static inline int request_firmware_direct(const
> struct firmware **fw,
> return -EINVAL;
> }
>
> +static inline void firmware_stat_init(struct firmware_stat *fwst)
> +{
> +}
> +
> +static inline unsigned long __firmware_stat_get(struct firmware_stat
> *fwst)
> +{
> + return -EINVAL;
> +}
> +
> +static inline void __firmware_stat_set(struct firmware_stat *fwst,
> + unsigned long status)
> +{
> +}
> +
> +static inline int __firmware_stat_wait(struct firmware_stat *fwst,
> + long timeout)
> +{
> + return -EINVAL;
> +}
> +
> +#define fw_loading_start(fwst)
> +#define fw_loading_done(fwst)
> +#define fw_loading_abort(fwst)
> +#define fw_loading_wait(fwst)
> +#define fw_loading_wait_timeout(fwst, timeout)
> +#define is_fw_loading(fwst) 0
> +#define is_fw_loading_done(fwst) 0
> +#define is_fw_loading_aborted(fwst) 0
> +
> #endif
> #endif
^ permalink raw reply
* Re: [RFC v0 4/8] Input: goodix: use firmware_stat instead of completion
From: Daniel Wagner @ 2016-07-28 13:10 UTC (permalink / raw)
To: Bastien Nocera, Daniel Wagner, Bjorn Andersson, Dmitry Torokhov,
Greg Kroah-Hartman, Johannes Berg, Kalle Valo, Ohad Ben-Cohen
Cc: linux-input, linux-kselftest, linux-wireless, linux-kernel,
irina.tirdea, octavian.purdila
In-Reply-To: <1469708450.6761.156.camel@hadess.net>
> Looking at the API, I really don't like the mixing of namespaces.
> Either it's fw_ or it's firmware_ but not a mix of both.
I agree, that was not really clever.
struct fw_loading { ... }
__fw_loading_*()
fw_loading_*()
Would that make more sense? firmware_loading_ as prefix is a bit long IMO.
> Also looks like fw_loading_start() would do nothing as the struct is
> likely zero initialised, and FW_STATUS_LOADING == 0. Maybe you need an
> UNSET enum member?
Good point, I cut a corner here a bit :). In the spirit of making things
more clear fw_loading_start() should be used.
> FW_STATUS_ABORT <- FW_STATUS_ABORTED, to match the adjective suffixes
> of the other members. Ditto fw_loading_abort() which doesn't abort
> firmware loading but sets the status.
Okay.
Thanks a lot for the feedback.
cheers,
daniel
^ permalink raw reply
* Re: [RFC v0 4/8] Input: goodix: use firmware_stat instead of completion
From: Bastien Nocera @ 2016-07-28 12:20 UTC (permalink / raw)
To: Daniel Wagner, Daniel Wagner, Bjorn Andersson, Dmitry Torokhov,
Greg Kroah-Hartman, Johannes Berg, Kalle Valo, Ohad Ben-Cohen
Cc: linux-input, linux-kselftest, linux-wireless, linux-kernel,
irina.tirdea, octavian.purdila
In-Reply-To: <f7dd0db5-af15-7310-ac9f-1034bdf4f592@bmw-carit.de>
On Thu, 2016-07-28 at 13:59 +0200, Daniel Wagner wrote:
> On 07/28/2016 01:22 PM, Bastien Nocera wrote:
> > On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote:
> > > From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> > >
> > > Loading firmware is an operation many drivers implement in
> > > various
> > > ways
> > > around the completion API. And most of them do it almost in the
> > > same
> > > way. Let's reuse the firmware_stat API which is used also by the
> > > firmware_class loader. Apart of streamlining the firmware loading
> > > states
> > > we also document it slightly better.
> > >
> > > Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> >
> > Irina added and tested that feature, so best for her to comment on
> > this, as I don't have any hardware that would use this feature.
>
> In case you have any comments on the API, let me know. I'll add Irina
> to
> the Cc list in the next version.
Looking at the API, I really don't like the mixing of namespaces.
Either it's fw_ or it's firmware_ but not a mix of both.
Also looks like fw_loading_start() would do nothing as the struct is
likely zero initialised, and FW_STATUS_LOADING == 0. Maybe you need an
UNSET enum member?
FW_STATUS_ABORT <- FW_STATUS_ABORTED, to match the adjective suffixes
of the other members. Ditto fw_loading_abort() which doesn't abort
firmware loading but sets the status.
Cheers
^ permalink raw reply
* Re: [RFC v0 4/8] Input: goodix: use firmware_stat instead of completion
From: Daniel Wagner @ 2016-07-28 11:59 UTC (permalink / raw)
To: Bastien Nocera, Daniel Wagner, Bjorn Andersson, Dmitry Torokhov,
Greg Kroah-Hartman, Johannes Berg, Kalle Valo, Ohad Ben-Cohen
Cc: linux-input, linux-kselftest, linux-wireless, linux-kernel,
irina.tirdea, octavian.purdila
In-Reply-To: <1469704952.6761.142.camel@hadess.net>
On 07/28/2016 01:22 PM, Bastien Nocera wrote:
> On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>>
>> Loading firmware is an operation many drivers implement in various
>> ways
>> around the completion API. And most of them do it almost in the same
>> way. Let's reuse the firmware_stat API which is used also by the
>> firmware_class loader. Apart of streamlining the firmware loading
>> states
>> we also document it slightly better.
>>
>> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
>
> Irina added and tested that feature, so best for her to comment on
> this, as I don't have any hardware that would use this feature.
In case you have any comments on the API, let me know. I'll add Irina to
the Cc list in the next version.
cheers,
daniel
^ permalink raw reply
* Re: [PATCH 2/3] staging: wilc1000: One function call less in mac_ioctl() after error detection
From: Julian Calaby @ 2016-07-28 12:02 UTC (permalink / raw)
To: SF Markus Elfring
Cc: linux-wireless, devel@driverdev.osuosl.org, Austin Shin,
Chris Park, Glen Lee, Greg Kroah-Hartman, Johnny Kim, Leo Kim,
Tony Cho, LKML, kernel-janitors, Julia Lawall
In-Reply-To: <590e0614-db41-a193-5463-9e3ad2630489@users.sourceforge.net>
Hi Marcus,
On Mon, Jul 25, 2016 at 6:22 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 24 Jul 2016 21:15:23 +0200
>
> The kfree() function was called in two cases by the mac_ioctl() function
> during error handling even if the passed variable did not contain a pointer
> for a valid data item.
>
> Improve this implementation detail by the introduction of another
> jump label.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
This is pointless micro-optimisation: kfree(NULL) is perfectly valid
and buff is either malloc'd memory or NULL when it's called.
Thanks,
--
Julian Calaby
Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
^ permalink raw reply
* Aw: Re: staging: wilc1000: Reduce scope for a few variables in mac_ioctl()
From: Lino Sanfilippo @ 2016-07-28 11:45 UTC (permalink / raw)
To: SF Markus Elfring
Cc: LKML, kernel-janitors, linux-wireless, devel, Greg Kroah-Hartman,
Steve Caldwell
In-Reply-To: <d8d9729c-6d65-a901-1b29-c0b031e143d2@users.sourceforge.net>
> Gesendet: Dienstag, 26. Juli 2016 um 08:25 Uhr
> Von: "SF Markus Elfring" <elfring@users.sourceforge.net>
>
> >> - if (strncasecmp(buff, "RSSI", length) == 0) {
> >> + if (strncasecmp(buff, "RSSI", 0) == 0) {
> >> + s8 rssi;
> >> +
> >
> > Um, please think a second about if it makes any sense at all to compare
> > zero chars of two strings.
>
> Under which circumstances should the variable "length" contain an other
> value than zero?
Which circumstances do "not any sense at all" imply?
>
> How can this open issue be fixed better?
The code is not too complicated and I think it is very obvious which value/variable
should be passed instead of 0. I suggest to fix this since it is indeed a bug, instead of
doing "micro optimizations" - which is the last thing that code in the staging area
needs (as IIRC you have already been told by others, including the staging maintainer).
Regards,
Lino
^ permalink raw reply
* Re: [RFC v0 4/8] Input: goodix: use firmware_stat instead of completion
From: Bastien Nocera @ 2016-07-28 11:22 UTC (permalink / raw)
To: Daniel Wagner, Bjorn Andersson, Dmitry Torokhov,
Greg Kroah-Hartman, Johannes Berg, Kalle Valo, Ohad Ben-Cohen
Cc: linux-input, linux-kselftest, linux-wireless, linux-kernel,
Daniel Wagner, irina.tirdea, octavian.purdila
In-Reply-To: <1469692512-16863-5-git-send-email-wagi@monom.org>
On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>
> Loading firmware is an operation many drivers implement in various
> ways
> around the completion API. And most of them do it almost in the same
> way. Let's reuse the firmware_stat API which is used also by the
> firmware_class loader. Apart of streamlining the firmware loading
> states
> we also document it slightly better.
>
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Irina added and tested that feature, so best for her to comment on
this, as I don't have any hardware that would use this feature.
Cheers
^ permalink raw reply
* Re: ath10k: Remove driver log suggesting QCA9887 support is experimental
From: Kalle Valo @ 2016-07-28 10:53 UTC (permalink / raw)
To: Mohammed Shafi Shajakhan
Cc: ath10k, mohammed, linux-wireless, Mohammed Shafi Shajakhan
In-Reply-To: <1469631896-9852-1-git-send-email-mohammed@qca.qualcomm.com>
Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com> wrote:
> From: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
>
> Support for QCA9887 is no longer experimental and if there are any issues
> we need to address them
>
> Signed-off-by: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
I'm planning to push this to 4.8 if no objections.
--
Sent by pwcli
https://patchwork.kernel.org/patch/9249941/
^ permalink raw reply
* Re: ath10k: fix get rx_status from htt context
From: Kalle Valo @ 2016-07-28 10:52 UTC (permalink / raw)
To: Ashok Raj Nagarajan; +Cc: ath10k, Ashok Raj Nagarajan, arnagara, linux-wireless
In-Reply-To: <1469551552-845-1-git-send-email-arnagara@qti.qualcomm.com>
Ashok Raj Nagarajan <arnagara@qti.qualcomm.com> wrote:
> On handling amsdu on rx path, get the rx_status from htt context. Without this
> fix, we are seeing warnings when running DBDC traffic like this.
>
> WARNING: CPU: 0 PID: 0 at net/mac80211/rx.c:4105 ieee80211_rx_napi+0x88/0x7d8 [mac80211]()
>
> [ 1715.878248] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 3.18.21 #1
> [ 1715.878273] [<c001d3f4>] (unwind_backtrace) from [<c001a4b0>] (show_stack+0x10/0x14)
> [ 1715.878293] [<c001a4b0>] (show_stack) from [<c01bee64>] (dump_stack+0x70/0xbc)
> [ 1715.878315] [<c01bee64>] (dump_stack) from [<c002a61c>] (warn_slowpath_common+0x64/0x88)
> [ 1715.878339] [<c002a61c>] (warn_slowpath_common) from [<c002a6d0>] (warn_slowpath_null+0x18/0x20)
> [ 1715.878395] [<c002a6d0>] (warn_slowpath_null) from [<bf4caa98>] (ieee80211_rx_napi+0x88/0x7d8 [mac80211])
> [ 1715.878474] [<bf4caa98>] (ieee80211_rx_napi [mac80211]) from [<bf568658>] (ath10k_htt_t2h_msg_handler+0xb48/0xbfc [ath10k_core])
> [ 1715.878535] [<bf568658>] (ath10k_htt_t2h_msg_handler [ath10k_core]) from [<bf568708>] (ath10k_htt_t2h_msg_handler+0xbf8/0xbfc [ath10k_core])
> [ 1715.878597] [<bf568708>] (ath10k_htt_t2h_msg_handler [ath10k_core]) from [<bf569160>] (ath10k_htt_txrx_compl_task+0xa54/0x1170 [ath10k_core])
> [ 1715.878639] [<bf569160>] (ath10k_htt_txrx_compl_task [ath10k_core]) from [<c002db14>] (tasklet_action+0xb4/0x130)
> [ 1715.878659] [<c002db14>] (tasklet_action) from [<c002d110>] (__do_softirq+0xe0/0x210)
> [ 1715.878678] [<c002d110>] (__do_softirq) from [<c002d4b4>] (irq_exit+0x84/0xe0)
> [ 1715.878700] [<c002d4b4>] (irq_exit) from [<c005a544>] (__handle_domain_irq+0x98/0xd0)
> [ 1715.878722] [<c005a544>] (__handle_domain_irq) from [<c00085f4>] (gic_handle_irq+0x38/0x5c)
> [ 1715.878741] [<c00085f4>] (gic_handle_irq) from [<c0009680>] (__irq_svc+0x40/0x74)
> [ 1715.878753] Exception stack(0xc05f9f50 to 0xc05f9f98)
> [ 1715.878767] 9f40: ffffffed 00000000 00399e1e c000a220
> [ 1715.878786] 9f60: 00000000 c05f6780 c05f8000 00000000 c05f5db8 ffffffed c05f8000 c04d1980
> [ 1715.878802] 9f80: 00000000 c05f9f98 c0018110 c0018114 60000013 ffffffff
> [ 1715.878822] [<c0009680>] (__irq_svc) from [<c0018114>] (arch_cpu_idle+0x2c/0x50)
> [ 1715.878844] [<c0018114>] (arch_cpu_idle) from [<c00530d4>] (cpu_startup_entry+0x108/0x234)
> [ 1715.878866] [<c00530d4>] (cpu_startup_entry) from [<c05c7be0>] (start_kernel+0x33c/0x3b8)
> [ 1715.878879] ---[ end trace 6d5e1cc0fef8ed6a ]---
> [ 1715.878899] ------------[ cut here ]------------
>
> Fixes: 18235664e7f9 ("ath10k: cleanup amsdu processing for rx indication")
> Signed-off-by: Ashok Raj Nagarajan <arnagara@qti.qualcomm.com>
I'm planning to push this to 4.8 if no objections.
--
Sent by pwcli
https://patchwork.kernel.org/patch/9248457/
^ permalink raw reply
* [RFC v0 5/8] ath9k_htc: use firmware_stat instead of completion
From: Daniel Wagner @ 2016-07-28 7:55 UTC (permalink / raw)
To: Bastien Nocera, Bjorn Andersson, Dmitry Torokhov,
Greg Kroah-Hartman, Johannes Berg, Kalle Valo, Ohad Ben-Cohen
Cc: linux-input, linux-kselftest, linux-wireless, linux-kernel,
Daniel Wagner
In-Reply-To: <1469692512-16863-1-git-send-email-wagi@monom.org>
From: Daniel Wagner <daniel.wagner@bmw-carit.de>
Loading firmware is an operation many drivers implement in various ways
around the completion API. And most of them do it almost in the same
way. Let's reuse the firmware_stat API which is used also by the
firmware_class loader. Apart of streamlining the firmware loading states
we also document it slightly better.
Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
drivers/net/wireless/ath/ath9k/hif_usb.c | 10 +++++-----
drivers/net/wireless/ath/ath9k/hif_usb.h | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
index e1c338c..0a05d68 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -1067,7 +1067,7 @@ static void ath9k_hif_usb_firmware_fail(struct hif_device_usb *hif_dev)
struct device *dev = &hif_dev->udev->dev;
struct device *parent = dev->parent;
- complete_all(&hif_dev->fw_done);
+ fw_loading_abort(hif_dev->fw_st);
if (parent)
device_lock(parent);
@@ -1192,7 +1192,7 @@ static void ath9k_hif_usb_firmware_cb(const struct firmware *fw, void *context)
release_firmware(fw);
hif_dev->flags |= HIF_USB_READY;
- complete_all(&hif_dev->fw_done);
+ fw_loading_done(hif_dev->fw_st);
return;
@@ -1287,7 +1287,7 @@ static int ath9k_hif_usb_probe(struct usb_interface *interface,
#endif
usb_set_intfdata(interface, hif_dev);
- init_completion(&hif_dev->fw_done);
+ firmware_stat_init(&hif_dev->fw_st);
ret = ath9k_hif_request_firmware(hif_dev, true);
if (ret)
@@ -1330,7 +1330,7 @@ static void ath9k_hif_usb_disconnect(struct usb_interface *interface)
if (!hif_dev)
return;
- wait_for_completion(&hif_dev->fw_done);
+ fw_loading_wait(hif_dev->fw_st);
if (hif_dev->flags & HIF_USB_READY) {
ath9k_htc_hw_deinit(hif_dev->htc_handle, unplugged);
@@ -1363,7 +1363,7 @@ static int ath9k_hif_usb_suspend(struct usb_interface *interface,
if (!(hif_dev->flags & HIF_USB_START))
ath9k_htc_suspend(hif_dev->htc_handle);
- wait_for_completion(&hif_dev->fw_done);
+ fw_loading_wait(hif_dev->fw_st);
if (hif_dev->flags & HIF_USB_READY)
ath9k_hif_usb_dealloc_urbs(hif_dev);
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.h b/drivers/net/wireless/ath/ath9k/hif_usb.h
index 7c2ef7e..0af9fe4 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.h
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.h
@@ -111,7 +111,7 @@ struct hif_device_usb {
const struct usb_device_id *usb_device_id;
const void *fw_data;
size_t fw_size;
- struct completion fw_done;
+ struct firmware_stat fw_st;
struct htc_target *htc_handle;
struct hif_usb_tx tx;
struct usb_anchor regout_submitted;
--
2.7.4
^ permalink raw reply related
* [RFC v0 2/8] selftests: firmware: do not clutter output
From: Daniel Wagner @ 2016-07-28 7:55 UTC (permalink / raw)
To: Bastien Nocera, Bjorn Andersson, Dmitry Torokhov,
Greg Kroah-Hartman, Johannes Berg, Kalle Valo, Ohad Ben-Cohen
Cc: linux-input, linux-kselftest, linux-wireless, linux-kernel,
Daniel Wagner
In-Reply-To: <1469692512-16863-1-git-send-email-wagi@monom.org>
From: Daniel Wagner <daniel.wagner@bmw-carit.de>
Some of the test are supposed to fail but we get a few ouptput lines:
./fw_filesystem.sh: line 51: printf: write error: Invalid argument
./fw_filesystem.sh: line 56: printf: write error: No such device
./fw_filesystem.sh: line 62: echo: write error: No such file or directory
Let's silence them so that the selftest output is clean if all is fine.
Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
tools/testing/selftests/firmware/fw_filesystem.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh
index 5c495ad..d8ac9ba 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -48,18 +48,18 @@ echo "ABCD0123" >"$FW"
NAME=$(basename "$FW")
-if printf '\000' >"$DIR"/trigger_request; then
+if printf '\000' >"$DIR"/trigger_request 2> /dev/null; then
echo "$0: empty filename should not succeed" >&2
exit 1
fi
-if printf '\000' >"$DIR"/trigger_async_request; then
+if printf '\000' >"$DIR"/trigger_async_request 2> /dev/null; then
echo "$0: empty filename should not succeed (async)" >&2
exit 1
fi
# Request a firmware that doesn't exist, it should fail.
-if echo -n "nope-$NAME" >"$DIR"/trigger_request; then
+if echo -n "nope-$NAME" >"$DIR"/trigger_request 2> /dev/null; then
echo "$0: firmware shouldn't have loaded" >&2
exit 1
fi
--
2.7.4
^ permalink raw reply related
* [RFC v0 3/8] firmware: Factor out firmware load helpers
From: Daniel Wagner @ 2016-07-28 7:55 UTC (permalink / raw)
To: Bastien Nocera, Bjorn Andersson, Dmitry Torokhov,
Greg Kroah-Hartman, Johannes Berg, Kalle Valo, Ohad Ben-Cohen
Cc: linux-input, linux-kselftest, linux-wireless, linux-kernel,
Daniel Wagner
In-Reply-To: <1469692512-16863-1-git-send-email-wagi@monom.org>
From: Daniel Wagner <daniel.wagner@bmw-carit.de>
Factor out the firmware loading synchronization code in order to allow
drivers to reuse it. This also documents more clearly what is
happening. This is especial useful the drivers which will be converted
afterwards. Not everyone has to come with yet another way to handle it.
We use swait instead completion. complete() would only wake up one
waiter, so complete_all() is used. complete_all() wakes max MAX_UINT/2
waiters which is plenty in all cases. Though withh swait we just wait
until the exptected status shows up and wake any waiter.
Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
drivers/base/firmware_class.c | 112 +++++++++++++++++++-----------------------
include/linux/firmware.h | 71 ++++++++++++++++++++++++++
2 files changed, 122 insertions(+), 61 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 773fc30..bf1ca70 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -30,6 +30,7 @@
#include <linux/syscore_ops.h>
#include <linux/reboot.h>
#include <linux/security.h>
+#include <linux/swait.h>
#include <generated/utsrelease.h>
@@ -85,11 +86,36 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
}
#endif
-enum {
- FW_STATUS_LOADING,
- FW_STATUS_DONE,
- FW_STATUS_ABORT,
-};
+
+#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
+
+static inline bool is_fw_sync_done(unsigned long status)
+{
+ return status == FW_STATUS_LOADED || status == FW_STATUS_ABORT;
+}
+
+int __firmware_stat_wait(struct firmware_stat *fwst,
+ long timeout)
+{
+ int err;
+ err = swait_event_interruptible_timeout(fwst->wq,
+ is_fw_sync_done(READ_ONCE(fwst->status)),
+ timeout);
+ if (err == 0 && fwst->status == FW_STATUS_ABORT)
+ return -ENOENT;
+
+ return err;
+}
+EXPORT_SYMBOL(__firmware_stat_wait);
+
+void __firmware_stat_set(struct firmware_stat *fwst, unsigned long status)
+{
+ WRITE_ONCE(fwst->status, status);
+ swake_up(&fwst->wq);
+}
+EXPORT_SYMBOL(__firmware_stat_set);
+
+#endif
static int loading_timeout = 60; /* In seconds */
@@ -138,9 +164,8 @@ struct firmware_cache {
struct firmware_buf {
struct kref ref;
struct list_head list;
- struct completion completion;
+ struct firmware_stat fwst;
struct firmware_cache *fwc;
- unsigned long status;
void *data;
size_t size;
#ifdef CONFIG_FW_LOADER_USER_HELPER
@@ -194,7 +219,7 @@ static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
kref_init(&buf->ref);
buf->fwc = fwc;
- init_completion(&buf->completion);
+ firmware_stat_init(&buf->fwst);
#ifdef CONFIG_FW_LOADER_USER_HELPER
INIT_LIST_HEAD(&buf->pending_list);
#endif
@@ -292,15 +317,6 @@ static const char * const fw_path[] = {
module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
-static void fw_finish_direct_load(struct device *device,
- struct firmware_buf *buf)
-{
- mutex_lock(&fw_lock);
- set_bit(FW_STATUS_DONE, &buf->status);
- complete_all(&buf->completion);
- mutex_unlock(&fw_lock);
-}
-
static int fw_get_filesystem_firmware(struct device *device,
struct firmware_buf *buf)
{
@@ -339,7 +355,7 @@ static int fw_get_filesystem_firmware(struct device *device,
}
dev_dbg(device, "direct-loading %s\n", buf->fw_id);
buf->size = size;
- fw_finish_direct_load(device, buf);
+ fw_loading_done(buf->fwst);
break;
}
__putname(path);
@@ -457,12 +473,11 @@ static void __fw_load_abort(struct firmware_buf *buf)
* There is a small window in which user can write to 'loading'
* between loading done and disappearance of 'loading'
*/
- if (test_bit(FW_STATUS_DONE, &buf->status))
+ if (is_fw_loading_done(buf->fwst))
return;
list_del_init(&buf->pending_list);
- set_bit(FW_STATUS_ABORT, &buf->status);
- complete_all(&buf->completion);
+ fw_loading_abort(buf->fwst);
}
static void fw_load_abort(struct firmware_priv *fw_priv)
@@ -475,9 +490,6 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
fw_priv->buf = NULL;
}
-#define is_fw_load_aborted(buf) \
- test_bit(FW_STATUS_ABORT, &(buf)->status)
-
static LIST_HEAD(pending_fw_head);
/* reboot notifier for avoid deadlock with usermode_lock */
@@ -577,7 +589,7 @@ static ssize_t firmware_loading_show(struct device *dev,
mutex_lock(&fw_lock);
if (fw_priv->buf)
- loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf->status);
+ loading = is_fw_loading(fw_priv->buf->fwst);
mutex_unlock(&fw_lock);
return sprintf(buf, "%d\n", loading);
@@ -632,23 +644,20 @@ static ssize_t firmware_loading_store(struct device *dev,
switch (loading) {
case 1:
/* discarding any previous partial load */
- if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) {
+ if (!is_fw_loading_done(fw_buf->fwst)) {
for (i = 0; i < fw_buf->nr_pages; i++)
__free_page(fw_buf->pages[i]);
vfree(fw_buf->pages);
fw_buf->pages = NULL;
fw_buf->page_array_size = 0;
fw_buf->nr_pages = 0;
- set_bit(FW_STATUS_LOADING, &fw_buf->status);
+ fw_loading_start(fw_buf->fwst);
}
break;
case 0:
- if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
+ if (is_fw_loading(fw_buf->fwst)) {
int rc;
- set_bit(FW_STATUS_DONE, &fw_buf->status);
- clear_bit(FW_STATUS_LOADING, &fw_buf->status);
-
/*
* Several loading requests may be pending on
* one same firmware buf, so let all requests
@@ -670,10 +679,11 @@ static ssize_t firmware_loading_store(struct device *dev,
*/
list_del_init(&fw_buf->pending_list);
if (rc) {
- set_bit(FW_STATUS_ABORT, &fw_buf->status);
+ fw_loading_abort(fw_buf->fwst);
written = rc;
+ } else {
+ fw_loading_done(fw_buf->fwst);
}
- complete_all(&fw_buf->completion);
break;
}
/* fallthrough */
@@ -681,7 +691,7 @@ static ssize_t firmware_loading_store(struct device *dev,
dev_err(dev, "%s: unexpected value (%d)\n", __func__, loading);
/* fallthrough */
case -1:
- fw_load_abort(fw_priv);
+ fw_loading_abort(fw_buf->fwst);
break;
}
out:
@@ -702,7 +712,7 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
mutex_lock(&fw_lock);
buf = fw_priv->buf;
- if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
+ if (!buf || is_fw_loading_done(buf->fwst)) {
ret_count = -ENODEV;
goto out;
}
@@ -799,7 +809,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
mutex_lock(&fw_lock);
buf = fw_priv->buf;
- if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
+ if (!buf || is_fw_loading_done(buf->fwst)) {
retval = -ENODEV;
goto out;
}
@@ -917,8 +927,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
timeout = MAX_JIFFY_OFFSET;
}
- retval = wait_for_completion_interruptible_timeout(&buf->completion,
- timeout);
+ retval = fw_loading_wait_timeout(buf->fwst, timeout);
if (retval == -ERESTARTSYS || !retval) {
mutex_lock(&fw_lock);
fw_load_abort(fw_priv);
@@ -927,7 +936,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
retval = 0;
}
- if (is_fw_load_aborted(buf))
+ if (is_fw_loading_aborted(buf->fwst))
retval = -EAGAIN;
else if (!buf->data)
retval = -ENOMEM;
@@ -986,26 +995,6 @@ static inline void kill_requests_without_uevent(void) { }
#endif /* CONFIG_FW_LOADER_USER_HELPER */
-
-/* wait until the shared firmware_buf becomes ready (or error) */
-static int sync_cached_firmware_buf(struct firmware_buf *buf)
-{
- int ret = 0;
-
- mutex_lock(&fw_lock);
- while (!test_bit(FW_STATUS_DONE, &buf->status)) {
- if (is_fw_load_aborted(buf)) {
- ret = -ENOENT;
- break;
- }
- mutex_unlock(&fw_lock);
- ret = wait_for_completion_interruptible(&buf->completion);
- mutex_lock(&fw_lock);
- }
- mutex_unlock(&fw_lock);
- return ret;
-}
-
/* prepare firmware and firmware_buf structs;
* return 0 if a firmware is already assigned, 1 if need to load one,
* or a negative error code
@@ -1039,7 +1028,8 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
firmware->priv = buf;
if (ret > 0) {
- ret = sync_cached_firmware_buf(buf);
+ ret = fw_loading_wait_timeout(buf->fwst,
+ firmware_loading_timeout());
if (!ret) {
fw_set_page_data(buf, firmware);
return 0; /* assigned */
@@ -1057,7 +1047,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
struct firmware_buf *buf = fw->priv;
mutex_lock(&fw_lock);
- if (!buf->size || is_fw_load_aborted(buf)) {
+ if (!buf->size || is_fw_loading_aborted(buf->fwst)) {
mutex_unlock(&fw_lock);
return -ENOENT;
}
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 5c41c5e..f584160 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -4,10 +4,17 @@
#include <linux/types.h>
#include <linux/compiler.h>
#include <linux/gfp.h>
+#include <linux/swait.h>
#define FW_ACTION_NOHOTPLUG 0
#define FW_ACTION_HOTPLUG 1
+enum {
+ FW_STATUS_LOADING,
+ FW_STATUS_LOADED,
+ FW_STATUS_ABORT,
+};
+
struct firmware {
size_t size;
const u8 *data;
@@ -17,6 +24,11 @@ struct firmware {
void *priv;
};
+struct firmware_stat {
+ unsigned long status;
+ struct swait_queue_head wq;
+};
+
struct module;
struct device;
@@ -49,6 +61,36 @@ int request_firmware_direct(const struct firmware **fw, const char *name,
struct device *device);
void release_firmware(const struct firmware *fw);
+
+static inline void firmware_stat_init(struct firmware_stat *fwst)
+{
+ init_swait_queue_head(&fwst->wq);
+}
+
+static inline unsigned long __firmware_stat_get(struct firmware_stat *fwst)
+{
+ return READ_ONCE(fwst->status);
+}
+void __firmware_stat_set(struct firmware_stat *fwst, unsigned long status);
+int __firmware_stat_wait(struct firmware_stat *fwst, long timeout);
+
+#define fw_loading_start(fwst) \
+ __firmware_stat_set(&fwst, FW_STATUS_LOADING)
+#define fw_loading_done(fwst) \
+ __firmware_stat_set(&fwst, FW_STATUS_LOADED)
+#define fw_loading_abort(fwst) \
+ __firmware_stat_set(&fwst, FW_STATUS_ABORT)
+#define fw_loading_wait(fwst) \
+ __firmware_stat_wait(&fwst, 0)
+#define fw_loading_wait_timeout(fwst, timeout) \
+ __firmware_stat_wait(&fwst, timeout)
+#define is_fw_loading(fwst) \
+ (__firmware_stat_get(&fwst) == FW_STATUS_LOADING)
+#define is_fw_loading_done(fwst) \
+ (__firmware_stat_get(&fwst) == FW_STATUS_LOADED)
+#define is_fw_loading_aborted(fwst) \
+ (__firmware_stat_get(&fwst) == FW_STATUS_ABORT)
+
#else
static inline int request_firmware(const struct firmware **fw,
const char *name,
@@ -75,5 +117,34 @@ static inline int request_firmware_direct(const struct firmware **fw,
return -EINVAL;
}
+static inline void firmware_stat_init(struct firmware_stat *fwst)
+{
+}
+
+static inline unsigned long __firmware_stat_get(struct firmware_stat *fwst)
+{
+ return -EINVAL;
+}
+
+static inline void __firmware_stat_set(struct firmware_stat *fwst,
+ unsigned long status)
+{
+}
+
+static inline int __firmware_stat_wait(struct firmware_stat *fwst,
+ long timeout)
+{
+ return -EINVAL;
+}
+
+#define fw_loading_start(fwst)
+#define fw_loading_done(fwst)
+#define fw_loading_abort(fwst)
+#define fw_loading_wait(fwst)
+#define fw_loading_wait_timeout(fwst, timeout)
+#define is_fw_loading(fwst) 0
+#define is_fw_loading_done(fwst) 0
+#define is_fw_loading_aborted(fwst) 0
+
#endif
#endif
--
2.7.4
^ permalink raw reply related
* [RFC v0 8/8] iwl4965: use firmware_stat instead of completion
From: Daniel Wagner @ 2016-07-28 7:55 UTC (permalink / raw)
To: Bastien Nocera, Bjorn Andersson, Dmitry Torokhov,
Greg Kroah-Hartman, Johannes Berg, Kalle Valo, Ohad Ben-Cohen
Cc: linux-input, linux-kselftest, linux-wireless, linux-kernel,
Daniel Wagner
In-Reply-To: <1469692512-16863-1-git-send-email-wagi@monom.org>
From: Daniel Wagner <daniel.wagner@bmw-carit.de>
Loading firmware is an operation many drivers implement in various ways
around the completion API. And most of them do it almost in the same
way. Let's reuse the firmware_stat API which is used also by the
firmware_class loader. Apart of streamlining the firmware loading states
we also document it slightly better.
Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
drivers/net/wireless/intel/iwlegacy/4965-mac.c | 8 ++++----
drivers/net/wireless/intel/iwlegacy/common.h | 3 ++-
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
index a91d170..d5e5808 100644
--- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
+++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
@@ -5005,7 +5005,7 @@ il4965_ucode_callback(const struct firmware *ucode_raw, void *context)
/* We have our copies now, allow OS release its copies */
release_firmware(ucode_raw);
- complete(&il->_4965.firmware_loading_complete);
+ fw_loading_done(il->_4965.fw_st);
return;
try_again:
@@ -5019,7 +5019,7 @@ err_pci_alloc:
IL_ERR("failed to allocate pci memory\n");
il4965_dealloc_ucode_pci(il);
out_unbind:
- complete(&il->_4965.firmware_loading_complete);
+ fw_loading_done(il->_4965.fw_st);
device_release_driver(&il->pci_dev->dev);
release_firmware(ucode_raw);
}
@@ -6678,7 +6678,7 @@ il4965_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
il_power_initialize(il);
- init_completion(&il->_4965.firmware_loading_complete);
+ firmware_stat_init(&il->_4965.fw_st);
err = il4965_request_firmware(il, true);
if (err)
@@ -6716,7 +6716,7 @@ il4965_pci_remove(struct pci_dev *pdev)
if (!il)
return;
- wait_for_completion(&il->_4965.firmware_loading_complete);
+ fw_loading_wait(il->_4965.fw_st);
D_INFO("*** UNLOAD DRIVER ***\n");
diff --git a/drivers/net/wireless/intel/iwlegacy/common.h b/drivers/net/wireless/intel/iwlegacy/common.h
index 726ede3..94af7b7 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.h
+++ b/drivers/net/wireless/intel/iwlegacy/common.h
@@ -32,6 +32,7 @@
#include <linux/leds.h>
#include <linux/wait.h>
#include <linux/io.h>
+#include <linux/firmware.h>
#include <net/mac80211.h>
#include <net/ieee80211_radiotap.h>
@@ -1357,7 +1358,7 @@ struct il_priv {
bool last_phy_res_valid;
u32 ampdu_ref;
- struct completion firmware_loading_complete;
+ struct firmware_stat fw_st;
/*
* chain noise reset and gain commands are the
--
2.7.4
^ permalink raw reply related
* [RFC v0 4/8] Input: goodix: use firmware_stat instead of completion
From: Daniel Wagner @ 2016-07-28 7:55 UTC (permalink / raw)
To: Bastien Nocera, Bjorn Andersson, Dmitry Torokhov,
Greg Kroah-Hartman, Johannes Berg, Kalle Valo, Ohad Ben-Cohen
Cc: linux-input, linux-kselftest, linux-wireless, linux-kernel,
Daniel Wagner
In-Reply-To: <1469692512-16863-1-git-send-email-wagi@monom.org>
From: Daniel Wagner <daniel.wagner@bmw-carit.de>
Loading firmware is an operation many drivers implement in various ways
around the completion API. And most of them do it almost in the same
way. Let's reuse the firmware_stat API which is used also by the
firmware_class loader. Apart of streamlining the firmware loading states
we also document it slightly better.
Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
drivers/input/touchscreen/goodix.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 240b16f..67158d3 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -47,7 +47,7 @@ struct goodix_ts_data {
u16 id;
u16 version;
const char *cfg_name;
- struct completion firmware_loading_complete;
+ struct firmware_stat fwst;
unsigned long irq_flags;
};
@@ -683,7 +683,7 @@ static void goodix_config_cb(const struct firmware *cfg, void *ctx)
err_release_cfg:
release_firmware(cfg);
- complete_all(&ts->firmware_loading_complete);
+ fw_loading_done(ts->fwst);
}
static int goodix_ts_probe(struct i2c_client *client,
@@ -705,7 +705,7 @@ static int goodix_ts_probe(struct i2c_client *client,
ts->client = client;
i2c_set_clientdata(client, ts);
- init_completion(&ts->firmware_loading_complete);
+ firmware_stat_init(&ts->fwst);
error = goodix_get_gpio_config(ts);
if (error)
@@ -766,7 +766,7 @@ static int goodix_ts_remove(struct i2c_client *client)
struct goodix_ts_data *ts = i2c_get_clientdata(client);
if (ts->gpiod_int && ts->gpiod_rst)
- wait_for_completion(&ts->firmware_loading_complete);
+ fw_loading_wait(ts->fwst);
return 0;
}
@@ -781,7 +781,7 @@ static int __maybe_unused goodix_suspend(struct device *dev)
if (!ts->gpiod_int || !ts->gpiod_rst)
return 0;
- wait_for_completion(&ts->firmware_loading_complete);
+ fw_loading_wait(ts->fwst);
/* Free IRQ as IRQ pin is used as output in the suspend sequence */
goodix_free_irq(ts);
--
2.7.4
^ permalink raw reply related
* [RFC v0 6/8] remoteproc: use firmware_stat instead of completion
From: Daniel Wagner @ 2016-07-28 7:55 UTC (permalink / raw)
To: Bastien Nocera, Bjorn Andersson, Dmitry Torokhov,
Greg Kroah-Hartman, Johannes Berg, Kalle Valo, Ohad Ben-Cohen
Cc: linux-input, linux-kselftest, linux-wireless, linux-kernel,
Daniel Wagner
In-Reply-To: <1469692512-16863-1-git-send-email-wagi@monom.org>
From: Daniel Wagner <daniel.wagner@bmw-carit.de>
Loading firmware is an operation many drivers implement in various ways
around the completion API. And most of them do it almost in the same
way. Let's reuse the firmware_stat API which is used also by the
firmware_class loader. Apart of streamlining the firmware loading states
we also document it slightly better.
Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
drivers/remoteproc/remoteproc_core.c | 10 +++++-----
drivers/soc/ti/wkup_m3_ipc.c | 2 +-
include/linux/remoteproc.h | 6 ++++--
3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index db3958b..3b158f7 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -936,7 +936,7 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
out:
release_firmware(fw);
/* allow rproc_del() contexts, if any, to proceed */
- complete_all(&rproc->firmware_loading_complete);
+ fw_loading_done(rproc->fw_st);
}
static int rproc_add_virtio_devices(struct rproc *rproc)
@@ -944,7 +944,7 @@ static int rproc_add_virtio_devices(struct rproc *rproc)
int ret;
/* rproc_del() calls must wait until async loader completes */
- init_completion(&rproc->firmware_loading_complete);
+ firmware_stat_init(&rproc->fw_st);
/*
* We must retrieve early virtio configuration info from
@@ -959,7 +959,7 @@ static int rproc_add_virtio_devices(struct rproc *rproc)
rproc, rproc_fw_config_virtio);
if (ret < 0) {
dev_err(&rproc->dev, "request_firmware_nowait err: %d\n", ret);
- complete_all(&rproc->firmware_loading_complete);
+ fw_loading_abort(rproc->fw_st);
}
return ret;
@@ -1089,7 +1089,7 @@ static int __rproc_boot(struct rproc *rproc, bool wait)
/* if rproc virtio is not yet configured, wait */
if (wait)
- wait_for_completion(&rproc->firmware_loading_complete);
+ fw_loading_wait(rproc->fw_st);
ret = rproc_fw_boot(rproc, firmware_p);
@@ -1447,7 +1447,7 @@ int rproc_del(struct rproc *rproc)
return -EINVAL;
/* if rproc is just being registered, wait */
- wait_for_completion(&rproc->firmware_loading_complete);
+ fw_loading_wait(rproc->fw_st);
/* clean up remote vdev entries */
list_for_each_entry_safe(rvdev, tmp, &rproc->rvdevs, node)
diff --git a/drivers/soc/ti/wkup_m3_ipc.c b/drivers/soc/ti/wkup_m3_ipc.c
index 8823cc8..14c6396 100644
--- a/drivers/soc/ti/wkup_m3_ipc.c
+++ b/drivers/soc/ti/wkup_m3_ipc.c
@@ -370,7 +370,7 @@ static void wkup_m3_rproc_boot_thread(struct wkup_m3_ipc *m3_ipc)
struct device *dev = m3_ipc->dev;
int ret;
- wait_for_completion(&m3_ipc->rproc->firmware_loading_complete);
+ fw_loading_wait(m3_ipc->rproc->fw_st);
init_completion(&m3_ipc->sync_complete);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 1c457a8..b8e7ff4 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -41,6 +41,7 @@
#include <linux/completion.h>
#include <linux/idr.h>
#include <linux/of.h>
+#include <linux/firmware.h>
/**
* struct resource_table - firmware resource table header
@@ -397,7 +398,7 @@ enum rproc_crash_type {
* @num_traces: number of trace buffers
* @carveouts: list of physically contiguous memory allocations
* @mappings: list of iommu mappings we initiated, needed on shutdown
- * @firmware_loading_complete: marks e/o asynchronous firmware loading
+ * @fw_sync: marks e/o asynchronous firmware loading
* @bootaddr: address of first instruction to boot rproc with (optional)
* @rvdevs: list of remote virtio devices
* @notifyids: idr for dynamically assigning rproc-wide unique notify ids
@@ -429,7 +430,7 @@ struct rproc {
int num_traces;
struct list_head carveouts;
struct list_head mappings;
- struct completion firmware_loading_complete;
+ struct firmware_stat fw_st;
u32 bootaddr;
struct list_head rvdevs;
struct idr notifyids;
@@ -479,6 +480,7 @@ struct rproc_vring {
* @vring: the vrings for this vdev
* @rsc_offset: offset of the vdev's resource entry
*/
+
struct rproc_vdev {
struct list_head node;
struct rproc *rproc;
--
2.7.4
^ permalink raw reply related
* [RFC v0 0/8] Reuse firmware loader helpers
From: Daniel Wagner @ 2016-07-28 7:55 UTC (permalink / raw)
To: Bastien Nocera, Bjorn Andersson, Dmitry Torokhov,
Greg Kroah-Hartman, Johannes Berg, Kalle Valo, Ohad Ben-Cohen
Cc: linux-input, linux-kselftest, linux-wireless, linux-kernel,
Daniel Wagner
From: Daniel Wagner <daniel.wagner@bmw-carit.de>
Hi,
While reviewing all the complete_all() users, I realized there is
recouring pattern how the completion API is used to synchronize the
stages of the firmware loading. Since firmware_class.c contains a
fairly complete implemetation for synching the loading, it worthwhile
to export it and reuse it in drivers, At the same time one
complete_all() user is gone which is a good thing for -rt (*).
The first 2 patches are bug fixes for the test script.
Patch 3 adds the new API, and the following patches update a few
drivers.
I haven't updated all the drivers because I wanted to see first if
this is going into the right direction. Since naming is a difficult, I
am more than please to pick a better name if you have one.
cheers,
daniel
(*) Under -rt waking all waiters via complete_all() is not good. If
the complete_all() call happens in IRQ context we have an unbound
latency there. Therefore the aim is to reduce the complete_all() users
and get rid of them where possible.
Daniel Wagner (8):
selftests: firmware: do not abort test too early
selftests: firmware: do not clutter output
firmware: Factor out firmware load helpers
Input: goodix: use firmware_stat instead of completion
ath9k_htc: use firmware_stat instead of completion
remoteproc: use firmware_stat instead of completion
Input: ims-pcu: use firmware_stat instead of completion
iwl4965: use firmware_stat instead of completion
drivers/base/firmware_class.c | 112 ++++++++++------------
drivers/input/misc/ims-pcu.c | 10 +-
drivers/input/touchscreen/goodix.c | 10 +-
drivers/net/wireless/ath/ath9k/hif_usb.c | 10 +-
drivers/net/wireless/ath/ath9k/hif_usb.h | 2 +-
drivers/net/wireless/intel/iwlegacy/4965-mac.c | 8 +-
drivers/net/wireless/intel/iwlegacy/common.h | 3 +-
drivers/remoteproc/remoteproc_core.c | 10 +-
drivers/soc/ti/wkup_m3_ipc.c | 2 +-
include/linux/firmware.h | 71 ++++++++++++++
include/linux/remoteproc.h | 6 +-
tools/testing/selftests/firmware/fw_filesystem.sh | 6 +-
tools/testing/selftests/firmware/fw_userhelper.sh | 2 +-
13 files changed, 158 insertions(+), 94 deletions(-)
--
2.7.4
^ permalink raw reply
* [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion
From: Daniel Wagner @ 2016-07-28 7:55 UTC (permalink / raw)
To: Bastien Nocera, Bjorn Andersson, Dmitry Torokhov,
Greg Kroah-Hartman, Johannes Berg, Kalle Valo, Ohad Ben-Cohen
Cc: linux-input, linux-kselftest, linux-wireless, linux-kernel,
Daniel Wagner
In-Reply-To: <1469692512-16863-1-git-send-email-wagi@monom.org>
From: Daniel Wagner <daniel.wagner@bmw-carit.de>
Loading firmware is an operation many drivers implement in various ways
around the completion API. And most of them do it almost in the same
way. Let's reuse the firmware_stat API which is used also by the
firmware_class loader. Apart of streamlining the firmware loading states
we also document it slightly better.
Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
drivers/input/misc/ims-pcu.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 9c0ea36..cda1fbf 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -109,7 +109,7 @@ struct ims_pcu {
u32 fw_start_addr;
u32 fw_end_addr;
- struct completion async_firmware_done;
+ struct firmware_stat fw_st;
struct ims_pcu_buttons buttons;
struct ims_pcu_gamepad *gamepad;
@@ -940,7 +940,7 @@ static void ims_pcu_process_async_firmware(const struct firmware *fw,
release_firmware(fw);
out:
- complete(&pcu->async_firmware_done);
+ fw_loading_done(pcu->fw_st);
}
/*********************************************************************
@@ -1967,7 +1967,7 @@ static int ims_pcu_init_bootloader_mode(struct ims_pcu *pcu)
ims_pcu_process_async_firmware);
if (error) {
/* This error is not fatal, let userspace have another chance */
- complete(&pcu->async_firmware_done);
+ fw_loading_abort(pcu->fw_st);
}
return 0;
@@ -1976,7 +1976,7 @@ static int ims_pcu_init_bootloader_mode(struct ims_pcu *pcu)
static void ims_pcu_destroy_bootloader_mode(struct ims_pcu *pcu)
{
/* Make sure our initial firmware request has completed */
- wait_for_completion(&pcu->async_firmware_done);
+ fw_loading_wait(pcu->fw_st);
}
#define IMS_PCU_APPLICATION_MODE 0
@@ -2000,7 +2000,7 @@ static int ims_pcu_probe(struct usb_interface *intf,
pcu->bootloader_mode = id->driver_info == IMS_PCU_BOOTLOADER_MODE;
mutex_init(&pcu->cmd_mutex);
init_completion(&pcu->cmd_done);
- init_completion(&pcu->async_firmware_done);
+ firmware_stat_init(&pcu->fw_st);
error = ims_pcu_parse_cdc_data(intf, pcu);
if (error)
--
2.7.4
^ permalink raw reply related
* [RFC v0 1/8] selftests: firmware: do not abort test too early
From: Daniel Wagner @ 2016-07-28 7:55 UTC (permalink / raw)
To: Bastien Nocera, Bjorn Andersson, Dmitry Torokhov,
Greg Kroah-Hartman, Johannes Berg, Kalle Valo, Ohad Ben-Cohen
Cc: linux-input, linux-kselftest, linux-wireless, linux-kernel,
Daniel Wagner
In-Reply-To: <1469692512-16863-1-git-send-email-wagi@monom.org>
From: Daniel Wagner <daniel.wagner@bmw-carit.de>
When running the test script you will get:
kselftest/firmware/fw_userhelper.sh: line 69: echo: write error: Resource temporarily unavailable
and stops right there. Because the script runs with the '-e' option
which will stop the script at any error.
We should stop there because we are trying to something wrong and get an
error reported back. Instead, ignore the error message and make sure we
do not stop the script by setting the last error code to true.
Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
tools/testing/selftests/firmware/fw_userhelper.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/firmware/fw_userhelper.sh b/tools/testing/selftests/firmware/fw_userhelper.sh
index b9983f8..2d244a7 100755
--- a/tools/testing/selftests/firmware/fw_userhelper.sh
+++ b/tools/testing/selftests/firmware/fw_userhelper.sh
@@ -66,7 +66,7 @@ NAME=$(basename "$FW")
# Test failure when doing nothing (timeout works).
echo 1 >/sys/class/firmware/timeout
-echo -n "$NAME" >"$DIR"/trigger_request
+echo -n "$NAME" >"$DIR"/trigger_request 2> /dev/null || true
if diff -q "$FW" /dev/test_firmware >/dev/null ; then
echo "$0: firmware was not expected to match" >&2
exit 1
--
2.7.4
^ permalink raw reply related
* rfkill bound to a switch
From: Kai Hendry @ 2016-07-28 8:02 UTC (permalink / raw)
To: linux-wireless
Hi there,
Since laptops typically have a toggle button for killing and enabled
wifi, how does one supposed to bind rfkill like a toggle?
I.e. knowing which state you are in so you can call {block,unblock}
accordingly?
Parsing event or list output seems non-trivial.
Kind regards,
^ permalink raw reply
* Re: PROBLEM: network data corruption (bisected to e5a4b0bb803b)
From: Al Viro @ 2016-07-28 1:22 UTC (permalink / raw)
To: alexmcwhirter
Cc: David Miller, rlwinm, chunkeey, linux-wireless, netdev,
linux-kernel, Al Viro
In-Reply-To: <8b3126f66186015956e0f8090fb70532@triadic.us>
On Wed, Jul 27, 2016 at 08:26:48PM -0400, alexmcwhirter@triadic.us wrote:
> I'm going to go ahead and say this is where my issue and the op's issue
> begin to branch apart from one another. He's seeing this on all incoming
> data, whereas i am only seeing it on ssl data and not on sun4v.
>
> At this point i would say data from my issue is only going to cloud this
> issue as they seem to be two completely different issues revolving around
> the same commit. If i come across any relevant data for x86_64 ill be sure
> to post it if this isn't resolved by then, but for now i'm going to refrain
> from submitting anything sparc related.
Which just might mean that we have *three* issues here -
(1) buggered __copy_to_user_inatomic() (and friends) on some sparcs
(2) your ssl-only corruption
(3) Alan's x86_64 corruption on plain TCP read - no ssl *or* sparc
anywhere, and no multi-segment recvmsg(). Which would strongly argue in
favour of some kind of copy_page_to_iter() breakage triggered when handling
a fragmented skb, as in (1). Except that I don't see anything similar in
x86_64 uaccess primitives...
^ permalink raw reply
* Re: PROBLEM: network data corruption (bisected to e5a4b0bb803b)
From: alexmcwhirter @ 2016-07-28 0:26 UTC (permalink / raw)
To: Al Viro
Cc: David Miller, rlwinm, chunkeey, linux-wireless, netdev,
linux-kernel, Al Viro
In-Reply-To: <20160728003102.GS2356@ZenIV.linux.org.uk>
On 2016-07-27 20:31, Al Viro wrote:
> On Wed, Jul 27, 2016 at 04:45:43PM -0700, David Miller wrote:
>
>> > I highly expect both my issue and OP's issue to revolve not around
>> > commit e5a4b0bb803b specifically, but around other code that no longer
>> > behaves as expected because of it.
>>
>> Indeed, and that fault address rounding bug occurs two other times
>> in arch/sparc/lib/user_fixup.c
>>
>> The mentioned patchwork patch should fix the bug and I'll get that
>> into my sparc tree, merged, and queued up for -stable ASAP.
>
> Plausible for sparc, but I don't see similar __copy_to_user_inatomic()
> bugs in case of x86_64...
I'm going to go ahead and say this is where my issue and the op's issue
begin to branch apart from one another. He's seeing this on all incoming
data, whereas i am only seeing it on ssl data and not on sun4v.
At this point i would say data from my issue is only going to cloud this
issue as they seem to be two completely different issues revolving
around the same commit. If i come across any relevant data for x86_64
ill be sure to post it if this isn't resolved by then, but for now i'm
going to refrain from submitting anything sparc related.
^ permalink raw reply
* Re: PROBLEM: network data corruption (bisected to e5a4b0bb803b)
From: Al Viro @ 2016-07-28 0:31 UTC (permalink / raw)
To: David Miller
Cc: alexmcwhirter, rlwinm, chunkeey, linux-wireless, netdev,
linux-kernel
In-Reply-To: <20160727.164543.1466564919313003461.davem@davemloft.net>
On Wed, Jul 27, 2016 at 04:45:43PM -0700, David Miller wrote:
> > I highly expect both my issue and OP's issue to revolve not around
> > commit e5a4b0bb803b specifically, but around other code that no longer
> > behaves as expected because of it.
>
> Indeed, and that fault address rounding bug occurs two other times
> in arch/sparc/lib/user_fixup.c
>
> The mentioned patchwork patch should fix the bug and I'll get that
> into my sparc tree, merged, and queued up for -stable ASAP.
Plausible for sparc, but I don't see similar __copy_to_user_inatomic()
bugs in case of x86_64...
^ permalink raw reply
* Re: PROBLEM: network data corruption (bisected to e5a4b0bb803b)
From: David Miller @ 2016-07-27 23:45 UTC (permalink / raw)
To: alexmcwhirter
Cc: rlwinm, viro, chunkeey, linux-wireless, netdev, linux-kernel
In-Reply-To: <87c4ed555bcb4229f2334f6f2bb69bcb@triadic.us>
From: alexmcwhirter@triadic.us
Date: Wed, 27 Jul 2016 19:02:40 -0400
> On 2016-07-27 14:04, alexmcwhirter@triadic.us wrote:
>> Just to add some more information to this, the corruption seems to
>> effect ssh as well.
>> Using a sun hme interface, occasionally upon an ssh connection it will
>> refuse to authenticate a client with either password or cert
>> authentication. Using wireshark to capture and decrypt the packets
>> between the two machines, the data coming from the server seems good,
>> but the data received by the server from the client is essentially
>> garbage. Note that the client is sending valid data, but the server is
>> corrupting it upon receipt. Closing the connection and starting a new
>> one will remedy the login issue, but you do occasionally see
>> corruption on the server side sporadically.
>> So far this only seems to occur on incoming data, outgoing data seems
>> fine.
>
> Also, there is another patch the references this commit on sparc64 at
> least.
>
> https://patchwork.kernel.org/patch/9221895/
>
> I highly expect both my issue and OP's issue to revolve not around
> commit e5a4b0bb803b specifically, but around other code that no longer
> behaves as expected because of it.
Indeed, and that fault address rounding bug occurs two other times
in arch/sparc/lib/user_fixup.c
The mentioned patchwork patch should fix the bug and I'll get that
into my sparc tree, merged, and queued up for -stable ASAP.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox