[PATCH v2] misc: bcm-vk: only support ttyVK if CONFIG_TTY is set

[PATCH v2] misc: bcm-vk: only support ttyVK if CONFIG_TTY is set

[Scott Branden]

Correct compile issue if CONFIG_TTY is not set by
only adding ttyVK devices if CONFIG_TTY is set.

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Scott Branden <scott.branden@broadcom.com>

---
Changes since v1:
Add function stubs rather than compiling out code
---
 drivers/misc/bcm-vk/Makefile     |  4 ++--
 drivers/misc/bcm-vk/bcm_vk.h     | 35 +++++++++++++++++++++++++++++---
 drivers/misc/bcm-vk/bcm_vk_dev.c |  3 +--
 drivers/misc/bcm-vk/bcm_vk_tty.c |  6 ++++++
 4 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/drivers/misc/bcm-vk/Makefile b/drivers/misc/bcm-vk/Makefile
index e4a1486f7209..8d81a734fcad 100644
--- a/drivers/misc/bcm-vk/Makefile
+++ b/drivers/misc/bcm-vk/Makefile
@@ -7,6 +7,6 @@ obj-$(CONFIG_BCM_VK) += bcm_vk.o
 bcm_vk-objs := \
 	bcm_vk_dev.o \
 	bcm_vk_msg.o \
-	bcm_vk_sg.o \
-	bcm_vk_tty.o
+	bcm_vk_sg.o
 
+bcm_vk-$(CONFIG_TTY) += bcm_vk_tty.o
diff --git a/drivers/misc/bcm-vk/bcm_vk.h b/drivers/misc/bcm-vk/bcm_vk.h
index 3f37c640a814..4a1d515374c7 100644
--- a/drivers/misc/bcm-vk/bcm_vk.h
+++ b/drivers/misc/bcm-vk/bcm_vk.h
@@ -258,7 +258,11 @@ enum pci_barno {
 	BAR_2
 };
 
+#ifdef CONFIG_TTY
 #define BCM_VK_NUM_TTY 2
+#else
+#define BCM_VK_NUM_TTY 0
+#endif
 
 struct bcm_vk_tty {
 	struct tty_port port;
@@ -366,11 +370,15 @@ struct bcm_vk {
 	struct miscdevice miscdev;
 	int devid; /* dev id allocated */
 
+#ifdef CONFIG_TTY
 	struct tty_driver *tty_drv;
 	struct timer_list serial_timer;
 	struct bcm_vk_tty tty[BCM_VK_NUM_TTY];
 	struct workqueue_struct *tty_wq_thread;
 	struct work_struct tty_wq_work;
+#else
+	struct bcm_vk_tty *tty;
+#endif
 
 	/* Reference-counting to handle file operations */
 	struct kref kref;
@@ -501,13 +509,34 @@ int bcm_vk_send_shutdown_msg(struct bcm_vk *vk, u32 shut_type,
 			     const pid_t pid, const u32 q_num);
 void bcm_to_v_q_doorbell(struct bcm_vk *vk, u32 q_num, u32 db_val);
 int bcm_vk_auto_load_all_images(struct bcm_vk *vk);
-int bcm_vk_tty_init(struct bcm_vk *vk, char *name);
-void bcm_vk_tty_exit(struct bcm_vk *vk);
-void bcm_vk_tty_terminate_tty_user(struct bcm_vk *vk);
 void bcm_vk_hb_init(struct bcm_vk *vk);
 void bcm_vk_hb_deinit(struct bcm_vk *vk);
 void bcm_vk_handle_notf(struct bcm_vk *vk);
 bool bcm_vk_drv_access_ok(struct bcm_vk *vk);
 void bcm_vk_set_host_alert(struct bcm_vk *vk, u32 bit_mask);
 
+#ifdef CONFIG_TTY
+int bcm_vk_tty_init(struct bcm_vk *vk, char *name);
+void bcm_vk_tty_exit(struct bcm_vk *vk);
+void bcm_vk_tty_terminate_tty_user(struct bcm_vk *vk);
+void bcm_vk_tty_wq_exit(struct bcm_vk *vk);
+#else
+static inline int bcm_vk_tty_init(struct bcm_vk *vk, char *name)
+{
+	return 0;
+}
+
+static inline void bcm_vk_tty_exit(struct bcm_vk *vk)
+{
+}
+
+static inline void bcm_vk_tty_terminate_tty_user(struct bcm_vk *vk)
+{
+}
+
+static inline void bcm_vk_tty_wq_exit(struct bcm_vk *vk)
+{
+}
+#endif /* CONFIG_TTY */
+
 #endif
diff --git a/drivers/misc/bcm-vk/bcm_vk_dev.c b/drivers/misc/bcm-vk/bcm_vk_dev.c
index c3d2bba68ef1..5d2030b67007 100644
--- a/drivers/misc/bcm-vk/bcm_vk_dev.c
+++ b/drivers/misc/bcm-vk/bcm_vk_dev.c
@@ -1580,8 +1580,7 @@ static void bcm_vk_remove(struct pci_dev *pdev)
 
 	cancel_work_sync(&vk->wq_work);
 	destroy_workqueue(vk->wq_thread);
-	cancel_work_sync(&vk->tty_wq_work);
-	destroy_workqueue(vk->tty_wq_thread);
+	bcm_vk_tty_wq_exit(vk);
 
 	for (i = 0; i < MAX_BAR; i++) {
 		if (vk->bar[i])
diff --git a/drivers/misc/bcm-vk/bcm_vk_tty.c b/drivers/misc/bcm-vk/bcm_vk_tty.c
index be3964949b63..4d02692ecfc7 100644
--- a/drivers/misc/bcm-vk/bcm_vk_tty.c
+++ b/drivers/misc/bcm-vk/bcm_vk_tty.c
@@ -331,3 +331,9 @@ void bcm_vk_tty_terminate_tty_user(struct bcm_vk *vk)
 			kill_pid(find_vpid(vktty->pid), SIGKILL, 1);
 	}
 }
+
+void bcm_vk_tty_wq_exit(struct bcm_vk *vk)
+{
+	cancel_work_sync(&vk->tty_wq_work);
+	destroy_workqueue(vk->tty_wq_thread);
+}
-- 
2.17.1

Re: [PATCH v2] misc: bcm-vk: only support ttyVK if CONFIG_TTY is set

[Randy Dunlap]

On 1/29/21 2:06 PM, Scott Branden wrote:
> Correct compile issue if CONFIG_TTY is not set by
> only adding ttyVK devices if CONFIG_TTY is set.
> 
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
> 
> ---
> Changes since v1:
> Add function stubs rather than compiling out code
> ---
>  drivers/misc/bcm-vk/Makefile     |  4 ++--
>  drivers/misc/bcm-vk/bcm_vk.h     | 35 +++++++++++++++++++++++++++++---
>  drivers/misc/bcm-vk/bcm_vk_dev.c |  3 +--
>  drivers/misc/bcm-vk/bcm_vk_tty.c |  6 ++++++
>  4 files changed, 41 insertions(+), 7 deletions(-)

Looks good. Thanks.

Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested


-- 
~Randy

Re: [PATCH v2] misc: bcm-vk: only support ttyVK if CONFIG_TTY is set

[Greg Kroah-Hartman]

On Fri, Jan 29, 2021 at 02:06:27PM -0800, Scott Branden wrote:
> Correct compile issue if CONFIG_TTY is not set by
> only adding ttyVK devices if CONFIG_TTY is set.
> 
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
> 
> ---
> Changes since v1:
> Add function stubs rather than compiling out code
> ---
>  drivers/misc/bcm-vk/Makefile     |  4 ++--
>  drivers/misc/bcm-vk/bcm_vk.h     | 35 +++++++++++++++++++++++++++++---
>  drivers/misc/bcm-vk/bcm_vk_dev.c |  3 +--
>  drivers/misc/bcm-vk/bcm_vk_tty.c |  6 ++++++
>  4 files changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/misc/bcm-vk/Makefile b/drivers/misc/bcm-vk/Makefile
> index e4a1486f7209..8d81a734fcad 100644
> --- a/drivers/misc/bcm-vk/Makefile
> +++ b/drivers/misc/bcm-vk/Makefile
> @@ -7,6 +7,6 @@ obj-$(CONFIG_BCM_VK) += bcm_vk.o
>  bcm_vk-objs := \
>  	bcm_vk_dev.o \
>  	bcm_vk_msg.o \
> -	bcm_vk_sg.o \
> -	bcm_vk_tty.o
> +	bcm_vk_sg.o
>  
> +bcm_vk-$(CONFIG_TTY) += bcm_vk_tty.o
> diff --git a/drivers/misc/bcm-vk/bcm_vk.h b/drivers/misc/bcm-vk/bcm_vk.h
> index 3f37c640a814..4a1d515374c7 100644
> --- a/drivers/misc/bcm-vk/bcm_vk.h
> +++ b/drivers/misc/bcm-vk/bcm_vk.h
> @@ -258,7 +258,11 @@ enum pci_barno {
>  	BAR_2
>  };
>  
> +#ifdef CONFIG_TTY
>  #define BCM_VK_NUM_TTY 2
> +#else
> +#define BCM_VK_NUM_TTY 0
> +#endif
>  
>  struct bcm_vk_tty {
>  	struct tty_port port;
> @@ -366,11 +370,15 @@ struct bcm_vk {
>  	struct miscdevice miscdev;
>  	int devid; /* dev id allocated */
>  
> +#ifdef CONFIG_TTY
>  	struct tty_driver *tty_drv;
>  	struct timer_list serial_timer;
>  	struct bcm_vk_tty tty[BCM_VK_NUM_TTY];
>  	struct workqueue_struct *tty_wq_thread;
>  	struct work_struct tty_wq_work;
> +#else
> +	struct bcm_vk_tty *tty;

Why do you still need this pointer?

And should you just have a separate config option for your tty driver
instead that depends on CONFIG_TTY?  Would you ever want to run this
driver without the tty portion?

Oh, and much better than the previous version, thanks for cleaning it
up.

thanks,

greg k-h

Re: [PATCH v2] misc: bcm-vk: only support ttyVK if CONFIG_TTY is set

[Scott Branden]

On 2021-01-30 12:55 a.m., Greg Kroah-Hartman wrote:
> On Fri, Jan 29, 2021 at 02:06:27PM -0800, Scott Branden wrote:
>> Correct compile issue if CONFIG_TTY is not set by
>> only adding ttyVK devices if CONFIG_TTY is set.
>>
>> Reported-by: Randy Dunlap <rdunlap@infradead.org>
>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>>
>> ---
>> Changes since v1:
>> Add function stubs rather than compiling out code
>> ---
>>  drivers/misc/bcm-vk/Makefile     |  4 ++--
>>  drivers/misc/bcm-vk/bcm_vk.h     | 35 +++++++++++++++++++++++++++++---
>>  drivers/misc/bcm-vk/bcm_vk_dev.c |  3 +--
>>  drivers/misc/bcm-vk/bcm_vk_tty.c |  6 ++++++
>>  4 files changed, 41 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/misc/bcm-vk/Makefile b/drivers/misc/bcm-vk/Makefile
>> index e4a1486f7209..8d81a734fcad 100644
>> --- a/drivers/misc/bcm-vk/Makefile
>> +++ b/drivers/misc/bcm-vk/Makefile
>> @@ -7,6 +7,6 @@ obj-$(CONFIG_BCM_VK) += bcm_vk.o
>>  bcm_vk-objs := \
>>  	bcm_vk_dev.o \
>>  	bcm_vk_msg.o \
>> -	bcm_vk_sg.o \
>> -	bcm_vk_tty.o
>> +	bcm_vk_sg.o
>>  
>> +bcm_vk-$(CONFIG_TTY) += bcm_vk_tty.o
>> diff --git a/drivers/misc/bcm-vk/bcm_vk.h b/drivers/misc/bcm-vk/bcm_vk.h
>> index 3f37c640a814..4a1d515374c7 100644
>> --- a/drivers/misc/bcm-vk/bcm_vk.h
>> +++ b/drivers/misc/bcm-vk/bcm_vk.h
>> @@ -258,7 +258,11 @@ enum pci_barno {
>>  	BAR_2
>>  };
>>  
>> +#ifdef CONFIG_TTY
>>  #define BCM_VK_NUM_TTY 2
>> +#else
>> +#define BCM_VK_NUM_TTY 0
>> +#endif
>>  
>>  struct bcm_vk_tty {
>>  	struct tty_port port;
>> @@ -366,11 +370,15 @@ struct bcm_vk {
>>  	struct miscdevice miscdev;
>>  	int devid; /* dev id allocated */
>>  
>> +#ifdef CONFIG_TTY
>>  	struct tty_driver *tty_drv;
>>  	struct timer_list serial_timer;
>>  	struct bcm_vk_tty tty[BCM_VK_NUM_TTY];
>>  	struct workqueue_struct *tty_wq_thread;
>>  	struct work_struct tty_wq_work;
>> +#else
>> +	struct bcm_vk_tty *tty;
> Why do you still need this pointer?
vk->tty is still in one location in bcm_vk_dev.c when installing the IRQ.
The loop is never executed as VK_MSIX_TTY_MAX = 0 when CONFIG_TTY is not defined.

I'll move setting vk-tty[i].irq_enabled into an inline function in the header file to clean this up.

    for (i = 0;
         (i < VK_MSIX_TTY_MAX) && (vk->num_irqs < irq);
         i++, vk->num_irqs++) {
        err = devm_request_irq(dev, pci_irq_vector(pdev, vk->num_irqs),
                       bcm_vk_tty_irqhandler,
                       IRQF_SHARED, DRV_MODULE_NAME, vk);
        if (err) {
            dev_err(dev, "failed request tty IRQ %d for MSIX %d\n",
                pdev->irq + vk->num_irqs, vk->num_irqs + 1);
            goto err_irq;
        }
        vk->tty[i].irq_enabled = true;
    }


> And should you just have a separate config option for your tty driver
> instead that depends on CONFIG_TTY?  Would you ever want to run this
> driver without the tty portion?
Yes, an additional config option could be added.
Looking at the code, it would simplify (a non-upstreamable) patch that allows the driver to run on
an ancient kernel where we compile out some features that don't work due to kernel api changes since then.

I hadn't added such a config as some are of the opinion having a full featured driver without config options is better.
For example, someone builds the driver without the feature enabled,
then someone needs to use the feature and the driver would need to be rebuilt.

Since it sounds like you are for such CONFIG options I will add it as it simplifies some legacy kernel support used in manufacturing.
> Oh, and much better than the previous version, thanks for cleaning it
> up.
Thanks - your comments do highlight some issues and we are learning from them.
>
> thanks,
>
> greg k-h