On Thu, Apr 02, 2026 at 02:11:23PM +0900, Kosugi Souta wrote:
> Signed-off-by: Kosugi Souta <k.souta0926(a)gmail.com>
> ---
> drivers/staging/greybus/Documentation/firmware/authenticate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/greybus/Documentation/firmware/authenticate.c b/drivers/staging/greybus/Documentation/firmware/authenticate.c
> index 3d2c6f88a138a..0ef88b7d24de0 100644
> --- a/drivers/staging/greybus/Documentation/firmware/authenticate.c
> +++ b/drivers/staging/greybus/Documentation/firmware/authenticate.c
> @@ -58,7 +58,7 @@ int main(int argc, char *argv[])
> goto close_fd;
> }
>
> - printf("UID received: 0x%llx\n", *(unsigned long long int *)(uid.uid));
> + printf("UID received: 0x%llx\n", *(unsigned long long *)(uid.uid));
>
> /* Get certificate */
> printf("Get IMS certificate\n");
> --
> 2.43.0
>
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- You did not specify a description of why the patch is needed, or
possibly, any description at all, in the email body. Please read the
section entitled "The canonical patch format" in the kernel file,
Documentation/process/submitting-patches.rst for what is needed in
order to properly describe the change.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
This small cleanup series replaces open-coded sprintf() usage in a set of
staging drivers with helpers intended for bounded and sysfs-safe formatting.
Patch 1 updates fbtft logging strings to use scnprintf().
Patch 2 converts Greybus sysfs show paths to sysfs_emit(), including
normalizing attributes that were missing a trailing newline.
Changes in v2:
- resend with corrected author/sender identity (yug.merabtene(a)gmail.com)
- no code changes compared to v1
Yug Merabtene (2):
staging: fbtft: use scnprintf() for log strings
staging: greybus: switch sysfs show paths to sysfs_emit()
drivers/staging/fbtft/fbtft-core.c | 8 +++++---
drivers/staging/greybus/arche-apb-ctrl.c | 12 ++++++------
drivers/staging/greybus/arche-platform.c | 10 +++++-----
drivers/staging/greybus/audio_manager_module.c | 12 ++++++------
drivers/staging/greybus/gbphy.c | 2 +-
drivers/staging/greybus/light.c | 4 ++--
drivers/staging/greybus/loopback.c | 14 +++++++-------
7 files changed, 32 insertions(+), 30 deletions(-)
--
2.34.1
hdlc_append() calls usleep_range() to wait for circular buffer space,
but it is called with tx_producer_lock (a spinlock) held via
hdlc_tx_frames() -> hdlc_append_tx_frame()/hdlc_append_tx_u8()/etc.
Sleeping while holding a spinlock is illegal and can trigger
"BUG: scheduling while atomic".
Fix this by moving the buffer-space wait out of hdlc_append() and into
hdlc_tx_frames(), before the spinlock is acquired. The new flow:
1. Pre-calculate the worst-case encoded frame length.
2. Wait (with sleep) outside the lock until enough space is available,
kicking the TX consumer work to drain the buffer.
3. Acquire the spinlock, re-verify space, and write the entire frame
atomically.
This ensures that sleeping only happens without any lock held, and
that frames are either fully enqueued or not written at all.
This bug is found by CodeQL static analysis tool (interprocedural
sleep-in-atomic query) and my code review.
Fixes: ec558bbfea67 ("greybus: Add BeaglePlay Linux Driver")
Cc: stable(a)vger.kernel.org
Cc: Ayush Singh <ayushdevel1325(a)gmail.com>
Cc: Johan Hovold <johan(a)kernel.org>
Cc: Alex Elder <elder(a)kernel.org>
Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
Signed-off-by: Weigang He <geoffreyhe2(a)gmail.com>
---
drivers/greybus/gb-beagleplay.c | 105 +++++++++++++++++++++++++++-----
1 file changed, 89 insertions(+), 16 deletions(-)
diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
index 87186f891a6ac..da1b9039fd2f3 100644
--- a/drivers/greybus/gb-beagleplay.c
+++ b/drivers/greybus/gb-beagleplay.c
@@ -242,30 +242,26 @@ static void hdlc_write(struct gb_beagleplay *bg)
}
/**
- * hdlc_append() - Queue HDLC data for sending.
+ * hdlc_append() - Queue a single HDLC byte for sending.
* @bg: beagleplay greybus driver
* @value: hdlc byte to transmit
*
- * Assumes that producer lock as been acquired.
+ * Caller must hold tx_producer_lock and must have ensured sufficient
+ * space in the circular buffer before calling (see hdlc_tx_frames()).
*/
static void hdlc_append(struct gb_beagleplay *bg, u8 value)
{
- int tail, head = bg->tx_circ_buf.head;
+ int head = bg->tx_circ_buf.head;
+ int tail = READ_ONCE(bg->tx_circ_buf.tail);
- while (true) {
- tail = READ_ONCE(bg->tx_circ_buf.tail);
-
- if (CIRC_SPACE(head, tail, TX_CIRC_BUF_SIZE) >= 1) {
- bg->tx_circ_buf.buf[head] = value;
+ lockdep_assert_held(&bg->tx_producer_lock);
+ if (WARN_ON_ONCE(CIRC_SPACE(head, tail, TX_CIRC_BUF_SIZE) < 1))
+ return;
- /* Finish producing HDLC byte */
- smp_store_release(&bg->tx_circ_buf.head,
- (head + 1) & (TX_CIRC_BUF_SIZE - 1));
- return;
- }
- dev_warn(&bg->sd->dev, "Tx circ buf full");
- usleep_range(3000, 5000);
- }
+ bg->tx_circ_buf.buf[head] = value;
+ /* Ensure buffer write is visible before advancing head. */
+ smp_store_release(&bg->tx_circ_buf.head,
+ (head + 1) & (TX_CIRC_BUF_SIZE - 1));
}
static void hdlc_append_escaped(struct gb_beagleplay *bg, u8 value)
@@ -313,13 +309,90 @@ static void hdlc_transmit(struct work_struct *work)
spin_unlock_bh(&bg->tx_consumer_lock);
}
+/**
+ * hdlc_encoded_length() - Calculate worst-case encoded length of an HDLC frame.
+ * @payloads: array of payload buffers
+ * @count: number of payloads
+ *
+ * Returns the maximum number of bytes needed in the circular buffer.
+ */
+static size_t hdlc_encoded_length(const struct hdlc_payload payloads[],
+ size_t count)
+{
+ size_t i, payload_len = 0;
+
+ for (i = 0; i < count; i++)
+ payload_len += payloads[i].len;
+
+ /*
+ * Worst case: every data byte needs escaping (doubles in size).
+ * data bytes = address(1) + control(1) + payload + crc(2)
+ * framing = opening flag(1) + closing flag(1)
+ */
+ return 2 + (1 + 1 + payload_len + 2) * 2;
+}
+
+#define HDLC_TX_BUF_WAIT_RETRIES 500
+#define HDLC_TX_BUF_WAIT_US_MIN 3000
+#define HDLC_TX_BUF_WAIT_US_MAX 5000
+
+/**
+ * hdlc_tx_frames() - Encode and queue an HDLC frame for transmission.
+ * @bg: beagleplay greybus driver
+ * @address: HDLC address field
+ * @control: HDLC control field
+ * @payloads: array of payload buffers
+ * @count: number of payloads
+ *
+ * Sleeps outside the spinlock until enough circular-buffer space is
+ * available, then verifies space under the lock and writes the entire
+ * frame atomically. Either a complete frame is enqueued or nothing is
+ * written, avoiding both sleeping in atomic context and partial frames.
+ */
static void hdlc_tx_frames(struct gb_beagleplay *bg, u8 address, u8 control,
const struct hdlc_payload payloads[], size_t count)
{
+ size_t needed = hdlc_encoded_length(payloads, count);
+ int retries = HDLC_TX_BUF_WAIT_RETRIES;
size_t i;
+ int head, tail;
+
+ /* Wait outside the lock for sufficient buffer space. */
+ while (retries--) {
+ /* Pairs with smp_store_release() in hdlc_append(). */
+ head = smp_load_acquire(&bg->tx_circ_buf.head);
+ tail = READ_ONCE(bg->tx_circ_buf.tail);
+
+ if (CIRC_SPACE(head, tail, TX_CIRC_BUF_SIZE) >= needed)
+ break;
+
+ /* Kick the consumer and sleep — no lock held. */
+ schedule_work(&bg->tx_work);
+ usleep_range(HDLC_TX_BUF_WAIT_US_MIN, HDLC_TX_BUF_WAIT_US_MAX);
+ }
+
+ if (retries < 0) {
+ dev_warn_ratelimited(&bg->sd->dev,
+ "Tx circ buf full, dropping frame\n");
+ return;
+ }
spin_lock(&bg->tx_producer_lock);
+ /*
+ * Re-check under the lock. Should not fail since
+ * tx_producer_lock serialises all producers and the
+ * consumer only frees space, but guard against it.
+ */
+ head = bg->tx_circ_buf.head;
+ tail = READ_ONCE(bg->tx_circ_buf.tail);
+ if (unlikely(CIRC_SPACE(head, tail, TX_CIRC_BUF_SIZE) < needed)) {
+ spin_unlock(&bg->tx_producer_lock);
+ dev_warn_ratelimited(&bg->sd->dev,
+ "Tx circ buf space lost, dropping frame\n");
+ return;
+ }
+
hdlc_append_tx_frame(bg);
hdlc_append_tx_u8(bg, address);
hdlc_append_tx_u8(bg, control);
--
2.34.1
Driver core holds a reference to the USB interface and its parent USB
device while the interface is bound to a driver and there is no need to
take additional references unless the structures are needed after
disconnect.
Drop the redundant device reference to reduce cargo culting, make it
easier to spot drivers where an extra reference is needed, and reduce
the risk of memory leaks when drivers fail to release it.
Signed-off-by: Johan Hovold <johan(a)kernel.org>
---
This one needs one more spin... Sorry about the mess up.
Johan
Changes in v3:
- drop the leftover usb_dev_put() in early error paths
Changes in v2:
- drop temporary udev variable as reported by the kernel test robot
(W=1 warning)
drivers/greybus/es2.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/greybus/es2.c b/drivers/greybus/es2.c
index 6ae0ac828afa..75b889fa21b4 100644
--- a/drivers/greybus/es2.c
+++ b/drivers/greybus/es2.c
@@ -772,7 +772,6 @@ static int check_urb_status(struct urb *urb)
static void es2_destroy(struct es2_ap_dev *es2)
{
- struct usb_device *udev;
struct urb *urb;
int i;
@@ -804,10 +803,7 @@ static void es2_destroy(struct es2_ap_dev *es2)
gb_hd_cport_release_reserved(es2->hd, ES2_CPORT_CDSI1);
gb_hd_cport_release_reserved(es2->hd, ES2_CPORT_CDSI0);
- udev = es2->usb_dev;
gb_hd_put(es2->hd);
-
- usb_put_dev(udev);
}
static void cport_in_callback(struct urb *urb)
@@ -1257,11 +1253,10 @@ static int ap_probe(struct usb_interface *interface,
bool bulk_in_found = false;
bool arpc_in_found = false;
- udev = usb_get_dev(interface_to_usbdev(interface));
+ udev = interface_to_usbdev(interface);
num_cports = apb_get_cport_count(udev);
if (num_cports < 0) {
- usb_put_dev(udev);
dev_err(&udev->dev, "Cannot retrieve CPort count: %d\n",
num_cports);
return num_cports;
@@ -1269,10 +1264,8 @@ static int ap_probe(struct usb_interface *interface,
hd = gb_hd_create(&es2_driver, &udev->dev, ES2_GBUF_MSG_SIZE_MAX,
num_cports);
- if (IS_ERR(hd)) {
- usb_put_dev(udev);
+ if (IS_ERR(hd))
return PTR_ERR(hd);
- }
es2 = hd_to_es2(hd);
es2->hd = hd;
--
2.52.0
In gbaudio_init_jack(), when setting SND_JACK_BTN_3 key, the error
message incorrectly says "Failed to set BTN_0". This should be
"Failed to set BTN_3" to match the button being configured.
Signed-off-by: Haoyu Lu <hechushiguitu666(a)gmail.com>
Reviewed-by: Johan Hovold <johan(a)kernel.org>
---
drivers/staging/greybus/audio_codec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
index 444c53b4e08d..720aa752e17e 100644
--- a/drivers/staging/greybus/audio_codec.c
+++ b/drivers/staging/greybus/audio_codec.c
@@ -781,7 +781,7 @@ static int gbaudio_init_jack(struct gbaudio_module_info *module,
ret = snd_jack_set_key(module->button.jack.jack, SND_JACK_BTN_3,
KEY_VOLUMEDOWN);
if (ret) {
- dev_err(module->dev, "Failed to set BTN_0\n");
+ dev_err(module->dev, "Failed to set BTN_3\n");
goto free_jacks;
}
}
--
2.17.1
In gbaudio_init_jack(), when setting SND_JACK_BTN_3 key, the error message
incorrectly says "Failed to set BTN_0". This should be "Failed to set BTN_3"
to match the button being configured.
Signed-off-by: Haoyu Lu <hechushiguitu666(a)gmail.com>
---
drivers/staging/greybus/audio_codec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
index 444c53b4e08d..720aa752e17e 100644
--- a/drivers/staging/greybus/audio_codec.c
+++ b/drivers/staging/greybus/audio_codec.c
@@ -781,7 +781,7 @@ static int gbaudio_init_jack(struct gbaudio_module_info *module,
ret = snd_jack_set_key(module->button.jack.jack, SND_JACK_BTN_3,
KEY_VOLUMEDOWN);
if (ret) {
- dev_err(module->dev, "Failed to set BTN_0\n");
+ dev_err(module->dev, "Failed to set BTN_3\n");
goto free_jacks;
}
}
--
2.17.1