diff mbox series

[net-next,v3,2/3] net: wwan: t7xx: Add sysfs attribute for device state machine

Message ID MEYP282MB2697CEBA4B69B0230089AA51BB9EA@MEYP282MB2697.AUSP282.PROD.OUTLOOK.COM (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: wwan: t7xx: Add fastboot interface | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1113 this patch: 1113
netdev/cc_maintainers warning 4 maintainers not CCed: linux-mediatek@lists.infradead.org angelogioacchino.delregno@collabora.com matthias.bgg@gmail.com linux-arm-kernel@lists.infradead.org
netdev/build_clang success Errors and warnings before: 1140 this patch: 1140
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1140 this patch: 1140
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 152 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jinjian Song Dec. 28, 2023, 9:44 a.m. UTC 
From: Jinjian Song <jinjian.song@fibocom.com>

Add support for userspace to get the device mode,
e.g., reset/ready/fastboot mode.

Signed-off-by: Jinjian Song <jinjian.song@fibocom.com>
---
v3:
 * no change 
v2:
 * optimizing using goto label in t7xx_pci_probe
---
 drivers/net/wwan/t7xx/t7xx_modem_ops.c     |  1 +
 drivers/net/wwan/t7xx/t7xx_pci.c           | 85 +++++++++++++++++++++-
 drivers/net/wwan/t7xx/t7xx_pci.h           | 11 +++
 drivers/net/wwan/t7xx/t7xx_state_monitor.c |  1 +
 4 files changed, 94 insertions(+), 4 deletions(-)

Comments

Jakub Kicinski Jan. 8, 2024, 3:19 p.m. UTC | #1 
On Thu, 28 Dec 2023 17:44:10 +0800 Jinjian Song wrote:
> From: Jinjian Song <jinjian.song@fibocom.com>
> 
> Add support for userspace to get the device mode,
> e.g., reset/ready/fastboot mode.

We need some info under Documentation/ describing the file / attr
and how it's supposed to be used.

> diff --git a/drivers/net/wwan/t7xx/t7xx_modem_ops.c b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
> index 24e7d491468e..ae4578905a8d 100644
> --- a/drivers/net/wwan/t7xx/t7xx_modem_ops.c
> +++ b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
> @@ -192,6 +192,7 @@ static irqreturn_t t7xx_rgu_isr_thread(int irq, void *data)
>  {
>  	struct t7xx_pci_dev *t7xx_dev = data;
>  
> +	atomic_set(&t7xx_dev->mode, T7XX_RESET);

Why is ->mode atomic? There seem to be no RMW operations on it.
You can use READ_ONCE() / WRITE_ONCE() on a normal integer.

>  	msleep(RGU_RESET_DELAY_MS);
>  	t7xx_reset_device_via_pmic(t7xx_dev);
>  	return IRQ_HANDLED;
> diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
> index 91256e005b84..d5f6a8638aba 100644
> --- a/drivers/net/wwan/t7xx/t7xx_pci.c
> +++ b/drivers/net/wwan/t7xx/t7xx_pci.c
> @@ -52,6 +52,68 @@
>  #define PM_RESOURCE_POLL_TIMEOUT_US	10000
>  #define PM_RESOURCE_POLL_STEP_US	100
>  
> +static ssize_t t7xx_mode_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct pci_dev *pdev;
> +	struct t7xx_pci_dev *t7xx_dev;
> +
> +	pdev = to_pci_dev(dev);
> +	t7xx_dev = pci_get_drvdata(pdev);
> +	if (!t7xx_dev)
> +		return -ENODEV;
> +
> +	atomic_set(&t7xx_dev->mode, T7XX_FASTBOOT_DL_SWITCHING);

This function doesn't use @buf at all. So whenever user does write()
to the file we set to SWITCHING? Shouldn't we narrow down the set
of accepted values to be able to add more functionality later?

> +	return count;
> +};

unnecessary semi-colon
Sergey Ryazanov Jan. 8, 2024, 9:37 p.m. UTC | #2 
On 28.12.2023 11:44, Jinjian Song wrote:

[skipped]

> +	switch (mode) {
> +	case T7XX_READY:
> +		return sprintf(buf, "T7XX_MODEM_READY\n");
> +	case T7XX_RESET:
> +		return sprintf(buf, "T7XX_MODEM_RESET\n");
> +	case T7XX_FASTBOOT_DL_SWITCHING:
> +		return sprintf(buf, "T7XX_MODEM_FASTBOOT_DL_SWITCHING\n");
> +	case T7XX_FASTBOOT_DL_MODE:
> +		return sprintf(buf, "T7XX_MODEM_FASTBOOT_DL_MODE\n");
> +	case T7XX_FASTBOOT_DUMP_MODE:
> +		return sprintf(buf, "T7XX_MODEM_FASTBOOT_DUMP_MODE\n");
> +	default:
> +		return sprintf(buf, "T7XX_UNKNOWN\n");

Out of curiosity, what the purpose of this common prefix "T7XX_MODEM_"? 
Do you have a plan to support more then T7xx modems?

And BTW, can we use a lighter method of string copying like strncpy()?

--
Sergey
Nelson, Shannon Jan. 9, 2024, 7:56 p.m. UTC | #3 
On 1/8/2024 1:37 PM, Sergey Ryazanov wrote:
> 
> On 28.12.2023 11:44, Jinjian Song wrote:
> 
> [skipped]
> 
>> +     switch (mode) {
>> +     case T7XX_READY:
>> +             return sprintf(buf, "T7XX_MODEM_READY\n");
>> +     case T7XX_RESET:
>> +             return sprintf(buf, "T7XX_MODEM_RESET\n");
>> +     case T7XX_FASTBOOT_DL_SWITCHING:
>> +             return sprintf(buf, "T7XX_MODEM_FASTBOOT_DL_SWITCHING\n");
>> +     case T7XX_FASTBOOT_DL_MODE:
>> +             return sprintf(buf, "T7XX_MODEM_FASTBOOT_DL_MODE\n");
>> +     case T7XX_FASTBOOT_DUMP_MODE:
>> +             return sprintf(buf, "T7XX_MODEM_FASTBOOT_DUMP_MODE\n");
>> +     default:
>> +             return sprintf(buf, "T7XX_UNKNOWN\n");
> 
> Out of curiosity, what the purpose of this common prefix "T7XX_MODEM_"?
> Do you have a plan to support more then T7xx modems?
> 
> And BTW, can we use a lighter method of string copying like strncpy()?

A quick note from the sidelines: better would be strscpy()
See 
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings

sln


> 
> -- 
> Sergey
>
diff mbox series

Patch

diff --git a/drivers/net/wwan/t7xx/t7xx_modem_ops.c b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
index 24e7d491468e..ae4578905a8d 100644
--- a/drivers/net/wwan/t7xx/t7xx_modem_ops.c
+++ b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
@@ -192,6 +192,7 @@  static irqreturn_t t7xx_rgu_isr_thread(int irq, void *data)
 {
 	struct t7xx_pci_dev *t7xx_dev = data;
 
+	atomic_set(&t7xx_dev->mode, T7XX_RESET);
 	msleep(RGU_RESET_DELAY_MS);
 	t7xx_reset_device_via_pmic(t7xx_dev);
 	return IRQ_HANDLED;
diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
index 91256e005b84..d5f6a8638aba 100644
--- a/drivers/net/wwan/t7xx/t7xx_pci.c
+++ b/drivers/net/wwan/t7xx/t7xx_pci.c
@@ -52,6 +52,68 @@ 
 #define PM_RESOURCE_POLL_TIMEOUT_US	10000
 #define PM_RESOURCE_POLL_STEP_US	100
 
+static ssize_t t7xx_mode_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct pci_dev *pdev;
+	struct t7xx_pci_dev *t7xx_dev;
+
+	pdev = to_pci_dev(dev);
+	t7xx_dev = pci_get_drvdata(pdev);
+	if (!t7xx_dev)
+		return -ENODEV;
+
+	atomic_set(&t7xx_dev->mode, T7XX_FASTBOOT_DL_SWITCHING);
+	return count;
+};
+
+static ssize_t t7xx_mode_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	enum t7xx_mode mode = T7XX_UNKNOWN;
+	struct pci_dev *pdev;
+	struct t7xx_pci_dev *t7xx_dev;
+
+	pdev = to_pci_dev(dev);
+	t7xx_dev = pci_get_drvdata(pdev);
+	if (!t7xx_dev)
+		return -ENODEV;
+
+	mode = atomic_read(&t7xx_dev->mode);
+	switch (mode) {
+	case T7XX_READY:
+		return sprintf(buf, "T7XX_MODEM_READY\n");
+
+	case T7XX_RESET:
+		return sprintf(buf, "T7XX_MODEM_RESET\n");
+
+	case T7XX_FASTBOOT_DL_SWITCHING:
+		return sprintf(buf, "T7XX_MODEM_FASTBOOT_DL_SWITCHING\n");
+
+	case T7XX_FASTBOOT_DL_MODE:
+		return sprintf(buf, "T7XX_MODEM_FASTBOOT_DL_MODE\n");
+
+	case T7XX_FASTBOOT_DUMP_MODE:
+		return sprintf(buf, "T7XX_MODEM_FASTBOOT_DUMP_MODE\n");
+
+	default:
+		return sprintf(buf, "T7XX_UNKNOWN\n");
+	}
+}
+
+static DEVICE_ATTR_RW(t7xx_mode);
+
+static struct attribute *t7xx_mode_attr[] = {
+	&dev_attr_t7xx_mode.attr,
+	NULL
+};
+
+static const struct attribute_group t7xx_mode_attribute_group = {
+	.attrs = t7xx_mode_attr,
+};
+
 enum t7xx_pm_state {
 	MTK_PM_EXCEPTION,
 	MTK_PM_INIT,		/* Device initialized, but handshake not completed */
@@ -729,16 +791,28 @@  static int t7xx_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	t7xx_pcie_mac_interrupts_dis(t7xx_dev);
 
+	ret = sysfs_create_group(&t7xx_dev->pdev->dev.kobj,
+				 &t7xx_mode_attribute_group);
+	if (ret)
+		goto err_md_exit;
+
 	ret = t7xx_interrupt_init(t7xx_dev);
-	if (ret) {
-		t7xx_md_exit(t7xx_dev);
-		return ret;
-	}
+	if (ret)
+		goto err_remove_group;
+
 
 	t7xx_pcie_mac_set_int(t7xx_dev, MHCCIF_INT);
 	t7xx_pcie_mac_interrupts_en(t7xx_dev);
 
 	return 0;
+
+err_remove_group:
+	sysfs_remove_group(&t7xx_dev->pdev->dev.kobj,
+			   &t7xx_mode_attribute_group);
+
+err_md_exit:
+	t7xx_md_exit(t7xx_dev);
+	return ret;
 }
 
 static void t7xx_pci_remove(struct pci_dev *pdev)
@@ -747,6 +821,9 @@  static void t7xx_pci_remove(struct pci_dev *pdev)
 	int i;
 
 	t7xx_dev = pci_get_drvdata(pdev);
+
+	sysfs_remove_group(&t7xx_dev->pdev->dev.kobj,
+			   &t7xx_mode_attribute_group);
 	t7xx_md_exit(t7xx_dev);
 
 	for (i = 0; i < EXT_INT_NUM; i++) {
diff --git a/drivers/net/wwan/t7xx/t7xx_pci.h b/drivers/net/wwan/t7xx/t7xx_pci.h
index f08f1ab74469..fcd44e2d0a46 100644
--- a/drivers/net/wwan/t7xx/t7xx_pci.h
+++ b/drivers/net/wwan/t7xx/t7xx_pci.h
@@ -43,6 +43,15 @@  struct t7xx_addr_base {
 
 typedef irqreturn_t (*t7xx_intr_callback)(int irq, void *param);
 
+enum t7xx_mode {
+	T7XX_UNKNOWN,
+	T7XX_READY,
+	T7XX_RESET,
+	T7XX_FASTBOOT_DL_SWITCHING,
+	T7XX_FASTBOOT_DL_MODE,
+	T7XX_FASTBOOT_DUMP_MODE
+};
+
 /* struct t7xx_pci_dev - MTK device context structure
  * @intr_handler: array of handler function for request_threaded_irq
  * @intr_thread: array of thread_fn for request_threaded_irq
@@ -59,6 +68,7 @@  typedef irqreturn_t (*t7xx_intr_callback)(int irq, void *param);
  * @md_pm_lock: protects PCIe sleep lock
  * @sleep_disable_count: PCIe L1.2 lock counter
  * @sleep_lock_acquire: indicates that sleep has been disabled
+ * @mode: indicates the device mode
  */
 struct t7xx_pci_dev {
 	t7xx_intr_callback	intr_handler[EXT_INT_NUM];
@@ -82,6 +92,7 @@  struct t7xx_pci_dev {
 #ifdef CONFIG_WWAN_DEBUGFS
 	struct dentry		*debugfs_dir;
 #endif
+	atomic_t		mode;
 };
 
 enum t7xx_pm_id {
diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.c b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
index 0bc97430211b..23f54226aba0 100644
--- a/drivers/net/wwan/t7xx/t7xx_state_monitor.c
+++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
@@ -272,6 +272,7 @@  static void fsm_routine_ready(struct t7xx_fsm_ctl *ctl)
 
 	ctl->curr_state = FSM_STATE_READY;
 	t7xx_fsm_broadcast_ready_state(ctl);
+	atomic_set(&md->t7xx_dev->mode, T7XX_READY);
 	t7xx_md_event_notify(md, FSM_READY);
 }