Re: [PATCH v5 1/1] ptp: ocp: add CPLD ISP support for ADVA TimeCard X1
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-07-22 18:44:22
Also in:
lkml
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. --- ptp: ocp: add CPLD ISP support for ADVA TimeCard X1 This patch adds sysfs interfaces (i2c_bus_ctrl and cpld_i2c_xfer) on the ADVA TimeCard X1 variant so userspace can program the Lattice MachXO3 CPLD over a shared I2C bus. A new mutex, adapter-number cache and response buffer are added to struct ptp_ocp for the passthrough path. A few questions inline.
quoted hunk ↗ jump to hunk
diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c index 35e911f1ad78..91fa06f0e44c 100644 --- a/drivers/ptp/ptp_ocp.c +++ b/drivers/ptp/ptp_ocp.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -4224,6 +4230,205 @@ static const struct ocp_attr_group art_timecard_groups[] = { { }, }; +/* + * i2c_bus_ctrl exposes the MicroBlaze I2C bus arbitration register.
[Low] Should Documentation/ABI/testing/sysfs-timecard grow entries for the two new attributes added here (i2c_bus_ctrl and cpld_i2c_xfer)? The other timecard sysfs files are described there, and cpld_i2c_xfer in particular defines a private binary protocol (header layout, allowed slave addresses, response-buffer semantics) that userspace cannot discover from the filesystem alone. [ ... ]
quoted hunk ↗ jump to hunk
@@ -4902,6 +5114,7 @@ ptp_ocp_detach(struct ptp_ocp *bp) clk_hw_unregister_fixed_rate(bp->i2c_clk); if (bp->n_irqs) pci_free_irq_vectors(bp->pdev); + mutex_destroy(&bp->tap_i2c_lock); device_unregister(&bp->dev); }
[Medium]
Is mutex_destroy() here always called on an initialised mutex?
Looking at ptp_ocp_probe() below, mutex_init(&bp->tap_i2c_lock) runs
only after pci_alloc_irq_vectors() succeeds:
err = pci_alloc_irq_vectors(pdev, 1, 17, PCI_IRQ_MSI | PCI_IRQ_MSIX);
if (err < 0) {
dev_err(&pdev->dev, "alloc_irq_vectors err: %d\n", err);
goto out;
}
bp->n_irqs = err;
pci_set_master(pdev);
mutex_init(&bp->tap_i2c_lock);
bp->tap_i2c_adap_nr = -1;
If pci_alloc_irq_vectors() fails, the "goto out" reaches
ptp_ocp_detach(), which now unconditionally calls
mutex_destroy(&bp->tap_i2c_lock) on a bp that was zero-initialised by
devlink_alloc().
With CONFIG_DEBUG_MUTEXES=y, mutex_destroy() checks lock->magic and
would splat via DEBUG_LOCKS_WARN_ON(lock->magic != lock) on that error
path.
Would it be simpler to move the mutex_init() (and the tap_i2c_adap_nr
= -1 assignment) earlier, e.g. right after ptp_ocp_device_init()
succeeds, so every path that reaches ptp_ocp_detach() sees an
initialised mutex?
quoted hunk ↗ jump to hunk
@@ -5093,6 +5306,14 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) bp->n_irqs = err; pci_set_master(pdev); + /* Initialise before ptp_ocp_register_resources() so that the I2C bus + * notifier (ptp_ocp_i2c_notifier_call) cannot fire and write + * tap_i2c_adap_nr before ptp_ocp_adva_board_init() overwrites it + * with -1, leaving the adapter number lost for the device lifetime. + */ + mutex_init(&bp->tap_i2c_lock); + bp->tap_i2c_adap_nr = -1; + err = ptp_ocp_register_resources(bp, id->driver_data); if (err) goto out;
[Low] Does this comment describe what the code actually does? The comment says the ordering matters because ptp_ocp_adva_board_init() would otherwise overwrite tap_i2c_adap_nr with -1. Looking at ptp_ocp_adva_board_init(), it only touches flash_start, eeprom_map, sma_op, signals_nr, freq_in_nr, fw_tag, fw_version, fw_cap and calls a couple of sub-init routines. The -1 assignment is done here in ptp_ocp_probe() directly, not in ptp_ocp_adva_board_init(). Should the comment be reworded to refer to the actual initialiser? [ ... ] -- pw-bot: cr