Dear Oliver,

Thank you for reviewing,

Oliver Neukum [off-list ref] 於 2025年6月4日 週三 下午6:11寫道：

quoted

+static void usb_int_callback(struct urb *urb)
+{
+     struct nct6694 *nct6694 = urb->context;
+     unsigned int *int_status = urb->transfer_buffer;
+     int ret;
+
+     switch (urb->status) {
+     case 0:
+             break;
+     case -ECONNRESET:
+     case -ENOENT:
+     case -ESHUTDOWN:
+             return;
+     default:
+             goto resubmit;
+     }
+
+     while (*int_status) {
+             int irq = __ffs(*int_status);
+
+             generic_handle_irq_safe(irq_find_mapping(nct6694->domain, irq));
+             *int_status &= ~BIT(irq);
+     }

Does modifying the byte have any benefit?

I will update the code in the next patch to use __le32 for the
variable to ensure proper endianness handling across architectures.

quoted

+resubmit:
+     ret = usb_submit_urb(urb, GFP_ATOMIC);
+     if (ret)
+             dev_warn(nct6694->dev, "Failed to resubmit urb, status %pe",  ERR_PTR(ret));
+}
+
+static void nct6694_irq_lock(struct irq_data *data)
+{
+     struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data);
+
+     mutex_lock(&nct6694->irq_lock);
+}

Why? Does this do anything but make it _harder_ to tell that you
cannot take the lock in interrupt?

I plan to remove nct6694_irq_lock() and nct6694_bus_sync_unlock(), and
instead add the spinlock directly inside the function like this:
static void nct6694_irq_enable(struct irq_data *data)
{
    ...
    spin_lock(&nct6694->irq_lock);
    nct6694->irq_enable |= BIT(hwirq);
    spin_unlock(&nct6694->irq_lock);
}

Do you think this approach is better?


Best regards,
Ming