Re: [PATCH v2 2/2] net: usb: Add Infineon XMM6260 Baseband modem support

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: usb: Add Infineon XMM6260 Baseband modem support

This patch adds a platform driver for the Infineon/Intel XMM6260
3G modem. The driver toggles GPIOs and a regulator, drives a pwrseq
provider, and uses the global USB notifier plus rfkill to react to
the modem's HSIC USB enumeration.

A few observations on the commit message and overall placement before
the inline comments:

[Low]
The commit message tail reads:

  The modem is usually connected via the application processor's USB
  line in HSIC mode; however, to work properly, the modem must control
  this line

The sentence stops there with no completion.  Since this is the central
justification for adding a custom driver, can the explanation of what
"control this line" entails and why an existing solution does not work
be added?

[Low]
The driver is placed under drivers/net/usb/ but does not implement a
net_device (no register_netdev / alloc_netdev / netdev_ops, no usbnet
binding, no tx/rx callbacks).  The dmesg in the commit message and the
Kconfig help confirm the modem enumerates as cdc_acm and that data
goes over ttyACM0 with a userspace DHCP client.

Would drivers/usb/misc/ or drivers/power/ be a more natural home, both
for routing the patch to the right maintainers and to avoid this
appearing alongside actual USB net drivers?

[Low]
Could the design be re-expressed as a pure pwrseq provider attached to
the USB device's hub-port DT node, letting the USB core gate
enumeration?  The current driver pulls in pwrseq for part of the work
but keeps regulator + GPIO toggling, an rfkill instance, a global USB
notifier, and an ad-hoc bitmap state machine alongside it.

> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
> index 52a5c0922c79..d54d8db752df 100644
> --- a/drivers/net/usb/Kconfig
> +++ b/drivers/net/usb/Kconfig
> @@ -642,4 +642,19 @@ config USB_RTL8153_ECM
>  	  CONFIG_USB_RTL8152 is not set, or the RTL8153 device is not
>  	  supported by r8152 driver.
>  
> +config USB_NET_XMM6260
> +	tristate "Infineon XMM626X Baseband HSPA/HSUPA modem"

[Low]
The title says "XMM626X" (suggesting a family), but the USB notifier
hard-codes a single VID:PID 0x1519:0x0020 for XMM6260 only.  Other
626X variants such as XMM6262 will not be matched.  Should the title
be narrowed to XMM6260, or the match table extended?

> +	depends on USB_CHIPIDEA && (RFKILL || RFKILL=n)

[Low]
Does the driver actually have a software dependency on USB_CHIPIDEA?
The XMM6260 driver does not call any chipidea-specific API; it only
reacts to USB enumeration via the generic USB notifier and toggles
GPIOs/regulators/pwrseq.  Tying this to USB_CHIPIDEA prevents using
the driver on other HSIC-/USB-capable SoCs even though nothing in the
code is HCD-specific.

[ ... ]

> diff --git a/drivers/net/usb/baseband-xmm6260.c b/drivers/net/usb/baseband-xmm6260.c
> new file mode 100644
> index 000000000000..6710e46f4170
> --- /dev/null
> +++ b/drivers/net/usb/baseband-xmm6260.c
> @@ -0,0 +1,378 @@

[ ... ]

> +/* bits [0:2] */
> +enum baseband_xmm_ipc_ap_wake_state {
> +	BASEBAND_XMM_IPC_AP_WAKE_IRQ_READY,
> +	BASEBAND_XMM_IPC_AP_WAKE_INIT1,
> +	BASEBAND_XMM_IPC_AP_WAKE_INIT2,
> +	BASEBAND_XMM_IPC_AP_WAKE_L,
> +	BASEBAND_XMM_IPC_AP_WAKE_H,
> +	BASEBAND_XMM_IPC_AP_WAKE_UNINIT,
> +	BASEBAND_XMM_IPC_AP_WAKE_MASK = 7,
> +};
> +
> +#define BASEBAND_XMM_IPC_AP_WAKE_MAX	3
> +
> +#define BASEBAND_XMM_STATE_POWERED	3 /* Tracks regulator state */
> +#define BASEBAND_XMM_STATE_PROTECTED	4 /* Prevents rfkill from access */
> +#define BASEBAND_XMM_STATE_PRESENT	5 /* Tracks USB device presence */
> +#define BASEBAND_XMM_STATE_POWEROFF	6 /* Prevents poweroff recursive call */
> +#define BASEBAND_XMM_STATE_MAX		8

[Low]
The ipc_ap_wake_state enum reaches UNINIT=5 and MASK=7, but
BASEBAND_XMM_IPC_AP_WAKE_MAX=3 caps the encode/decode loops at three
bits.  Bits 3..6 of priv->state are then repurposed as orthogonal
STATE_POWERED / PROTECTED / PRESENT / POWEROFF flags.  Nothing
asserts that the wake-state value fits in three bits, so a future
enum entry past 7, or a typo widening WAKE_MAX, would silently
overwrite the STATE_* flags.  Could a BUILD_BUG_ON or static_assert
guard this invariant?

The L / H / UNINIT / MASK enumerators are also dead today; they only
appear at their declarations.  The block comment above the enum
describes a multi-state protocol with a retry cycle, but the IRQ
handler implements only IRQ_READY and INIT1.  Should the unused
values be removed, or should the handler be completed?

[ ... ]

> +static void set_ipc_ap_wake(struct baseband_xmm_data *priv,
> +			    enum baseband_xmm_ipc_ap_wake_state state)
> +{
> +	for (int i = 0; i < BASEBAND_XMM_IPC_AP_WAKE_MAX; i++)
> +		if (state & BIT(i))
> +			set_bit(i, priv->state);
> +		else
> +			clear_bit(i, priv->state);
> +}
> +
> +static void baseband_xmm_reset(struct baseband_xmm_data *priv)
> +{
> +	int ret;
> +
> +	set_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
> +
> +	if (!test_bit(BASEBAND_XMM_STATE_POWERED, priv->state)) {
> +		ret = regulator_enable(priv->vbat_supply);
> +		if (ret)
> +			dev_err(priv->dev,
> +				"failed to enable vbat power supply\n");
> +
> +		set_bit(BASEBAND_XMM_STATE_POWERED, priv->state);
> +	}

[High]
On regulator_enable() failure, ret is logged but STATE_POWERED is
still set unconditionally.  When baseband_xmm_poweroff() later
observes STATE_POWERED it will call regulator_disable() without a
matching enable, which trips WARN_ON in the regulator core for a
negative use_count and corrupts the disable count for every other
consumer of vbat.  Should the set_bit() be moved inside an "if (!ret)"
branch, or should the function bail out on enable failure?

> +
> +	gpiod_set_value_cansleep(priv->enable_gpio, 0);
> +	msleep(50);
> +
> +	gpiod_set_value_cansleep(priv->reset_gpio, 1);
> +	msleep(200);
> +	gpiod_set_value_cansleep(priv->reset_gpio, 0);
> +
> +	msleep(50);
> +
> +	/* falling edge trigger to CP */
> +	gpiod_set_value_cansleep(priv->enable_gpio, 1);
> +	usleep_range(50, 100);
> +	gpiod_set_value_cansleep(priv->enable_gpio, 0);
> +	msleep(20);
> +}
> +
> +static void baseband_xmm_poweroff(struct baseband_xmm_data *priv)
> +{
> +	/*
> +	 * The test_bit check prevents poweroff from being recursively
> +	 * called during USB device deregistration. USB device
> +	 * deregistration can be triggered by the driver by calling this
> +	 * function or by some external event. The first case will cause
> +	 * a recursive call by the notifier if not handled, while the
> +	 * second case requires this call to handle the USB controller
> +	 * properly.
> +	 */
> +	if (test_bit(BASEBAND_XMM_STATE_POWEROFF, priv->state))
> +		return;
> +
> +	set_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
> +	set_bit(BASEBAND_XMM_STATE_POWEROFF, priv->state);
> +
> +	pwrseq_power_off(priv->pwrseq);
> +	gpiod_set_value_cansleep(priv->reset_gpio, 1);
> +
> +	if (test_bit(BASEBAND_XMM_STATE_POWERED, priv->state)) {
> +		regulator_disable(priv->vbat_supply);
> +		clear_bit(BASEBAND_XMM_STATE_POWERED, priv->state);
> +	}
> +	set_ipc_ap_wake(priv, BASEBAND_XMM_IPC_AP_WAKE_IRQ_READY);
> +
> +	clear_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
> +	clear_bit(BASEBAND_XMM_STATE_PRESENT, priv->state);
> +	clear_bit(BASEBAND_XMM_STATE_POWEROFF, priv->state);
> +}

[High]
Are STATE_PROTECTED / STATE_POWEROFF / STATE_PRESENT being used here
as ad-hoc locks rather than as state?  The pattern is set on entry,
cleared on exit:

    if (test_bit(STATE_POWEROFF, priv->state))
        return;
    set_bit(STATE_PROTECTED, ...);
    set_bit(STATE_POWEROFF, ...);
    ...
    clear_bit(STATE_PROTECTED, ...);
    clear_bit(STATE_PRESENT, ...);
    clear_bit(STATE_POWEROFF, ...);

test_bit() followed by set_bit() is not atomic, so two callers (the
USB_DEVICE_REMOVE notifier on one CPU, the rfkill set_block callback,
the delayed work, or remove() on another) can both observe POWEROFF=0
and both proceed, calling pwrseq_power_off() twice and (when
STATE_POWERED is set) regulator_disable() twice.

The same concern applies to baseband_xmm_set_block(), which reads
STATE_PROTECTED and STATE_PRESENT outside of any lock, and to the
three separate set_bit/clear_bit operations inside set_ipc_ap_wake()
that the IRQ handler reads back via three separate test_bit() calls
(potential torn read across bits 0..2).

The networking subsystem documentation calls this pattern out
explicitly: "any pattern where a flag, atomic variable, or bit
operation appears to guard a section of code rather than express
state ... is a bug".  Could a real mutex (or spinlock for the IRQ
handler reads) be used instead?

> +
> +static int baseband_xmm_usb_notifier_call(struct notifier_block *nb,
> +					  unsigned long action, void *data)
> +{
> +	struct baseband_xmm_data *priv =
> +		container_of(nb, struct baseband_xmm_data, nb);
> +	struct usb_device *udev;
> +	u16 product, vendor;
> +
> +	if (action == USB_BUS_ADD || action == USB_BUS_REMOVE)
> +		return NOTIFY_OK;
> +
> +	udev = data;
> +	product = le16_to_cpu(udev->descriptor.idProduct);
> +	vendor = le16_to_cpu(udev->descriptor.idVendor);
> +
> +	switch (action) {
> +	case USB_DEVICE_ADD:
> +		/* Infineon XMM6260 ID 1519:0020 */
> +		if (vendor == BASEBAND_VENDOR_ID_COMNEON &&
> +		    product == BASEBAND_PRODUCT_ID_XMM6260) {
> +			cancel_delayed_work_sync(&priv->modem_work);
> +			clear_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
> +			set_bit(BASEBAND_XMM_STATE_PRESENT, priv->state);
> +		}
> +		break;

[Low]
Match is purely on VID:PID via the global USB notifier, with no
binding between this platform_device instance and the USB device that
enumerates.  If two XMM6260 modems are present in the system, both
driver instances react to either modem's add/remove.  Would binding a
USB device driver to that VID:PID, or routing the lifecycle through a
pwrseq attached to the hub-port node, give a stable per-instance
association?

> +
> +	case USB_DEVICE_REMOVE:
> +		/* Infineon XMM6260 ID 1519:0020 */
> +		if (vendor == BASEBAND_VENDOR_ID_COMNEON &&
> +		    product == BASEBAND_PRODUCT_ID_XMM6260)
> +			baseband_xmm_poweroff(priv);
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}

[ ... ]

> +static irqreturn_t baseband_hostwake_interrupt(int irq, void *dev_id)
> +{
> +	struct baseband_xmm_data *priv = dev_id;
> +	int state = gpiod_get_value(priv->ipc_ap_gpio);

[Medium]
This handler is registered with devm_request_threaded_irq(..., NULL,
&baseband_hostwake_interrupt, IRQF_ONESHOT | irqflags, ...), so it
runs in threaded (sleepable) context.  Should this read use
gpiod_get_value_cansleep() instead of gpiod_get_value()?  If ap-wake
is sourced from a sleeping GPIO controller (I2C/SPI expander, PMIC),
gpiod_get_value() will trip the WARN_ON(desc->gdev->can_sleep) in
gpiolib on every interrupt.  All other GPIO accesses in the driver
already use the *_cansleep variants.

> +
> +	switch (get_ipc_ap_wake(priv)) {
> +	case BASEBAND_XMM_IPC_AP_WAKE_IRQ_READY:
> +		if (!state) {
> +			set_ipc_ap_wake(priv, BASEBAND_XMM_IPC_AP_WAKE_INIT1);
> +			pwrseq_power_on(priv->pwrseq);
> +		}
> +		break;
> +
> +	case BASEBAND_XMM_IPC_AP_WAKE_INIT1:
> +		if (state) {
> +			set_ipc_ap_wake(priv, BASEBAND_XMM_IPC_AP_WAKE_INIT2);
> +			schedule_delayed_work(&priv->modem_work,
> +					      msecs_to_jiffies(BASEBAND_XMM_INIT_DELAY));
> +		}
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return IRQ_HANDLED;
> +}

[Medium]
Is the polarity of the IRQ_READY -> INIT1 transition consistent with
the comment above the enum?  The block comment states:

  AP wake interrupt keeps low util CP starts to initiate its HSIC hw.
  AP wake interrupt goes up during CP HSIC init
  (BASEBAND_XMM_IPC_AP_WAKE_INIT1 state)

So entering INIT1 corresponds to a rising edge (line goes high).  But
the handler enters INIT1 only when "!state" (line is low) and that's
where it calls pwrseq_power_on().  Either the comment is describing
the protocol incorrectly or the conditional checks the wrong polarity;
if the comment is accurate, the modem would never power on.  Could
this be clarified?

[Low]
For the non-DT path, irqflags is set to
IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING and the handler decides
which transition occurred by re-reading the GPIO.  The value seen in
the handler is the current level, not necessarily the level that
triggered the IRQ.  A coalesced fast L->H->L sequence (or a noisy
edge) can cause the handler to read a state that does not correspond
to the triggering edge, missing the INIT1 -> INIT2 transition or
jumping over states.  Should each edge be latched (e.g. by remembering
the previous level) instead of re-sampled?

> +
> +static int baseband_xmm_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	bitmap_zero(priv->state, BASEBAND_XMM_STATE_MAX);
> +	INIT_DELAYED_WORK(&priv->modem_work, baseband_xmm_work);
> +
> +	priv->irq = platform_get_irq(pdev, 0);
> +	if (priv->irq < 0)
> +		return dev_err_probe(dev, priv->irq, "failed to get IRQ\n");
> +
> +	/*
> +	 * Systems using device tree should set up interrupt via DT,
> +	 * the rest will use the default edge both interrupt.
> +	 */
> +	irqflags = dev->of_node ? 0 : IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
> +
> +	ret = devm_request_threaded_irq(dev, priv->irq, NULL,
> +					&baseband_hostwake_interrupt,
> +					IRQF_ONESHOT | irqflags,
> +					"modem-hostwake", priv);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to register IRQ %d\n", priv->irq);
> +
> +	priv->rfkill_dev = rfkill_alloc("xmm-modem", dev, RFKILL_TYPE_WWAN,
> +					&baseband_xmm_rfkill_ops, priv);
> +	if (!priv->rfkill_dev)
> +		return -ENOMEM;
> +
> +	ret = rfkill_register(priv->rfkill_dev);
> +	if (ret) {
> +		rfkill_destroy(priv->rfkill_dev);
> +		return dev_err_probe(dev, ret,
> +				     "failed to register WWAN rfkill\n");
> +	}

[High]
Once devm_request_threaded_irq() succeeds, the IRQ is live and the
handler can already step through IRQ_READY -> INIT1 (calling
pwrseq_power_on()) and on the next edge INIT1 -> INIT2, where it does:

    schedule_delayed_work(&priv->modem_work,
                          msecs_to_jiffies(BASEBAND_XMM_INIT_DELAY));

If rfkill_alloc() returns NULL or rfkill_register() fails, probe
returns an error.  devres releases the IRQ but does not cancel the
queued delayed work, and the devm_kzalloc()'d priv is then freed.
Five seconds later baseband_xmm_work() runs and dereferences priv
(priv->pwrseq, priv->state, priv->reset_gpio, priv->vbat_supply).

Should the error paths after devm_request_threaded_irq() call
cancel_delayed_work_sync(&priv->modem_work), or should the cancel be
registered via devm_add_action_or_reset()?

> +
> +	priv->nb.notifier_call = baseband_xmm_usb_notifier_call;
> +	usb_register_notify(&priv->nb);
> +
> +	return 0;
> +}

[High]
The ordering here is rfkill_register() first, usb_register_notify()
second.  rfkill_register() schedules sync_work, which runs the
set_block callback asynchronously.  Since this driver does not set
rfkill->persistent, the default state is unblocked, so set_block(false)
calls baseband_xmm_reset() -> regulator_enable() + GPIO toggling, and
the IRQ handler can step through IRQ_READY -> INIT1 -> INIT2 and
pwrseq_power_on() the modem, which then enumerates on USB.

If usb_register_notify() has not yet run when USB enumeration
completes, the USB_DEVICE_ADD notification is missed: STATE_PRESENT
is never set, the cancel_delayed_work_sync() in the notifier never
runs, and the 5-second watchdog work then calls baseband_xmm_poweroff()
on a working modem.

Should usb_register_notify() be called before rfkill_register()?

> +
> +static void baseband_xmm_remove(struct platform_device *pdev)
> +{
> +	struct baseband_xmm_data *priv = platform_get_drvdata(pdev);
> +
> +	rfkill_unregister(priv->rfkill_dev);
> +	rfkill_destroy(priv->rfkill_dev);
> +
> +	disable_irq(priv->irq);
> +	cancel_delayed_work_sync(&priv->modem_work);
> +
> +	usb_unregister_notify(&priv->nb);
> +	baseband_xmm_poweroff(priv);
> +}

[Medium]
Between cancel_delayed_work_sync() and usb_unregister_notify(), a
USB_DEVICE_REMOVE notification can still be in flight on another CPU
and call baseband_xmm_poweroff().  usb_unregister_notify() then waits
for that callback to complete, and immediately afterwards remove()
itself calls baseband_xmm_poweroff().  Combined with the
ad-hoc-locking concern above, this can result in pwrseq_power_off()
running twice and possibly regulator_disable() running twice.

Would unregistering the notifier first (before disable_irq /
cancel_delayed_work_sync) close that window?

[ ... ]

> +module_platform_driver(baseband_xmm_driver);
> +
> +MODULE_AUTHOR("Svyatolsav Ryhel <clamor95@gmail.com>");

[Low]
The name spelling here (Svyatolsav) does not match the file copyright
header or the patch's Signed-off-by ("Svyatoslav"); the 's' and 'l'
are transposed.

> +MODULE_DESCRIPTION("Baseband Infineon XMM6260 driver");
> +MODULE_LICENSE("GPL");
-- 
pw-bot: cr