Hello everyone. This is my first patch to linux kernel. Once I get the
hang of this process, I will be sending patches form GSoC 2023 greybus
driver. Feel free to tell me if I am doing something wrong here
Ayush Singh (1):
Remove extra newline
drivers/greybus/es2.c | 1 -
1 file changed, 1 deletion(-)
--
2.41.0
A few ALSA control API helpers like snd_ctl_rename(), snd_ctl_remove()
and snd_ctl_find_*() suppose the callers taking card->controls_rwsem.
But it's error-prone and fragile. This patch set tries to change
those API functions to take the card->controls>rwsem internally by
themselves, so that the drivers don't need to take care of lockings.
After applying this patch set, only a couple of places still touch
card->controls_rwsem (which are OK-ish as they need for traversing the
control linked list).
Takashi
===
Takashi Iwai (11):
ALSA: control: Take card->controls_rwsem in snd_ctl_rename()
staging: greybus: audio_helper: Use snd_ctl_remove_id()
ASoC: atmel: mchp-pdmc: Use snd_ctl_remove_id()
ALSA: control: Take controls_rwsem lock in snd_ctl_remove()
ALSA: control: Add lockdep warning to internal functions
ASoC: sigmadsp: Simplify with snd_ctl_activate_id()
staging: greybus: Avoid abusing controls_rwsem
ALSA: control: Make snd_ctl_find_id() argument const
ALSA: control: Introduce unlocked version for snd_ctl_find_*() helpers
ALSA: control: Take lock in snd_ctl_find_id() and snd_ctl_find_numid()
ALSA: emu10k1: Go back and simplify with snd_ctl_find_id()
drivers/staging/greybus/audio_codec.c | 18 ++--
drivers/staging/greybus/audio_codec.h | 1 +
drivers/staging/greybus/audio_helper.c | 20 +---
include/sound/control.h | 6 +-
sound/core/control.c | 126 ++++++++++++++++++++-----
sound/core/control_compat.c | 2 +-
sound/core/control_led.c | 2 +-
sound/core/jack.c | 2 -
sound/core/oss/mixer_oss.c | 10 +-
sound/core/pcm.c | 2 -
sound/isa/sb/emu8000.c | 2 -
sound/isa/sb/sb16_csp.c | 2 -
sound/pci/emu10k1/emufx.c | 5 -
sound/pci/hda/hda_codec.c | 2 -
sound/soc/atmel/mchp-pdmc.c | 12 +--
sound/soc/codecs/sigmadsp.c | 25 +----
sound/soc/soc-topology.c | 3 -
17 files changed, 129 insertions(+), 111 deletions(-)
--
2.35.3
The variables gb_tty->port.close_delay and gb_tty->port.closing_wait are
ofter accessed together while holding the lock gb_tty->port.mutex. Here is
an example in set_serial_info():
mutex_lock(&gb_tty->port.mutex);
...
gb_tty->port.close_delay = close_delay;
gb_tty->port.closing_wait = closing_wait;
...
mutex_unlock(&gb_tty->port.mutex);
However, they are accessed without holding the lock gb_tty->port.mutex when
are accessed in get_serial_info():
ss->close_delay = jiffies_to_msecs(gb_tty->port.close_delay) / 10;
ss->closing_wait =
gb_tty->port.closing_wait == ASYNC_CLOSING_WAIT_NONE ?
ASYNC_CLOSING_WAIT_NONE :
jiffies_to_msecs(gb_tty->port.closing_wait) / 10;
In my opinion, this may be a harmful race, because ss->close_delay can be
inconsistent with ss->closing_wait if gb_tty->port.close_delay and
gb_tty->port.closing_wait are updated by another thread after the
assignment to ss->close_delay.
Besides, the select operator may return wrong value if
gb_tty->port.closing_wait is updated right after the condition is
calculated.
To fix this possible data-inconsistency caused by data race, a lock and
unlock pair is added when accessing different fields of gb_tty->port.
Reported-by: BassCheck <bass(a)buaa.edu.cn>
Signed-off-by: Tuo Li <islituo(a)gmail.com>
---
drivers/staging/greybus/uart.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
index 20a34599859f..b8875517ea6a 100644
--- a/drivers/staging/greybus/uart.c
+++ b/drivers/staging/greybus/uart.c
@@ -596,12 +596,14 @@ static int get_serial_info(struct tty_struct *tty,
{
struct gb_tty *gb_tty = tty->driver_data;
+ mutex_lock(&gb_tty->port.mutex);
ss->line = gb_tty->minor;
ss->close_delay = jiffies_to_msecs(gb_tty->port.close_delay) / 10;
ss->closing_wait =
gb_tty->port.closing_wait == ASYNC_CLOSING_WAIT_NONE ?
ASYNC_CLOSING_WAIT_NONE :
jiffies_to_msecs(gb_tty->port.closing_wait) / 10;
+ mutex_unlock(&gb_tty->port.mutex);
return 0;
}
--
2.34.1
BACKGROUND
==========
When multiple work items are queued to a workqueue, their execution order
doesn't match the queueing order. They may get executed in any order and
simultaneously. When fully serialized execution - one by one in the queueing
order - is needed, an ordered workqueue should be used which can be created
with alloc_ordered_workqueue().
However, alloc_ordered_workqueue() was a later addition. Before it, an
ordered workqueue could be obtained by creating an UNBOUND workqueue with
@max_active==1. This originally was an implementation side-effect which was
broken by 4c16bd327c74 ("workqueue: restore WQ_UNBOUND/max_active==1 to be
ordered"). Because there were users that depended on the ordered execution,
5c0338c68706 ("workqueue: restore WQ_UNBOUND/max_active==1 to be ordered")
made workqueue allocation path to implicitly promote UNBOUND workqueues w/
@max_active==1 to ordered workqueues.
While this has worked okay, overloading the UNBOUND allocation interface
this way creates other issues. It's difficult to tell whether a given
workqueue actually needs to be ordered and users that legitimately want a
min concurrency level wq unexpectedly gets an ordered one instead. With
planned UNBOUND workqueue updates to improve execution locality and more
prevalence of chiplet designs which can benefit from such improvements, this
isn't a state we wanna be in forever.
This patch series audits all callsites that create an UNBOUND workqueue w/
@max_active==1 and converts them to alloc_ordered_workqueue() as necessary.
WHAT TO LOOK FOR
================
The conversions are from
alloc_workqueue(WQ_UNBOUND | flags, 1, args..)
to
alloc_ordered_workqueue(flags, args...)
which don't cause any functional changes. If you know that fully ordered
execution is not ncessary, please let me know. I'll drop the conversion and
instead add a comment noting the fact to reduce confusion while conversion
is in progress.
If you aren't fully sure, it's completely fine to let the conversion
through. The behavior will stay exactly the same and we can always
reconsider later.
As there are follow-up workqueue core changes, I'd really appreciate if the
patch can be routed through the workqueue tree w/ your acks. Thanks.
Signed-off-by: Tejun Heo <tj(a)kernel.org>
Cc: Johan Hovold <johan(a)kernel.org>
Cc: Alex Elder <elder(a)kernel.org>
Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
Cc: greybus-dev(a)lists.linaro.org
---
drivers/greybus/connection.c | 4 ++--
drivers/greybus/svc.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/greybus/connection.c b/drivers/greybus/connection.c
index e3799a53a193..9c88861986c8 100644
--- a/drivers/greybus/connection.c
+++ b/drivers/greybus/connection.c
@@ -187,8 +187,8 @@ _gb_connection_create(struct gb_host_device *hd, int hd_cport_id,
spin_lock_init(&connection->lock);
INIT_LIST_HEAD(&connection->operations);
- connection->wq = alloc_workqueue("%s:%d", WQ_UNBOUND, 1,
- dev_name(&hd->dev), hd_cport_id);
+ connection->wq = alloc_ordered_workqueue("%s:%d", 0, dev_name(&hd->dev),
+ hd_cport_id);
if (!connection->wq) {
ret = -ENOMEM;
goto err_free_connection;
diff --git a/drivers/greybus/svc.c b/drivers/greybus/svc.c
index 16cced80867a..0d7e749174a4 100644
--- a/drivers/greybus/svc.c
+++ b/drivers/greybus/svc.c
@@ -1318,7 +1318,7 @@ struct gb_svc *gb_svc_create(struct gb_host_device *hd)
if (!svc)
return NULL;
- svc->wq = alloc_workqueue("%s:svc", WQ_UNBOUND, 1, dev_name(&hd->dev));
+ svc->wq = alloc_ordered_workqueue("%s:svc", 0, dev_name(&hd->dev));
if (!svc->wq) {
kfree(svc);
return NULL;
--
2.40.0
In gb_camera_capture(), it does not check the value of settings
before dereferencing it. And gb_camera_debugfs_capture calls
gb_camera_capture with the 6th parameter settings as NULL.
Fix this by checking the value of setting at the starting of
gb_camera_capture.
Fixes: 3265edaf0d70 ("greybus: Add driver for the camera class protocol")
Signed-off-by: Dongliang Mu <dzm91(a)hust.edu.cn>
---
drivers/staging/greybus/camera.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/greybus/camera.c b/drivers/staging/greybus/camera.c
index cdbb42cd413b..5a4b26e7f645 100644
--- a/drivers/staging/greybus/camera.c
+++ b/drivers/staging/greybus/camera.c
@@ -659,7 +659,7 @@ static int gb_camera_capture(struct gb_camera *gcam, u32 request_id,
size_t req_size;
int ret;
- if (settings_size > GB_CAMERA_MAX_SETTINGS_SIZE)
+ if (settings_size > GB_CAMERA_MAX_SETTINGS_SIZE || !settings)
return -EINVAL;
req_size = sizeof(*req) + settings_size;
--
2.39.2