Correct compile issue if CONFIG_TTY is not set by
only adding ttyVK devices if CONFIG_BCM_VK_TTY is set.
Reported-by: Randy Dunlap <[email protected]>
Signed-off-by: Scott Branden <[email protected]>
---
Changes since v2:
- add CONFIG_BCM_VK_TTY
- add function and stub for bcm_vk_tty_set_irq_enabled
Changes since v1:
- add function stubs rather than compiling out code
---
drivers/misc/bcm-vk/Kconfig | 16 ++++++++++++
drivers/misc/bcm-vk/Makefile | 4 +--
drivers/misc/bcm-vk/bcm_vk.h | 42 +++++++++++++++++++++++++++++---
drivers/misc/bcm-vk/bcm_vk_dev.c | 5 ++--
drivers/misc/bcm-vk/bcm_vk_tty.c | 6 +++++
5 files changed, 65 insertions(+), 8 deletions(-)
diff --git a/drivers/misc/bcm-vk/Kconfig b/drivers/misc/bcm-vk/Kconfig
index 052f6f28b540..16ce98c964b8 100644
--- a/drivers/misc/bcm-vk/Kconfig
+++ b/drivers/misc/bcm-vk/Kconfig
@@ -15,3 +15,19 @@ config BCM_VK
accelerators via /dev/bcm-vk.N devices.
If unsure, say N.
+
+if BCM_VK
+
+config BCM_VK_TTY
+ bool "Enable ttyVK"
+ depends on TTY
+ default y
+ help
+ Select this option to enable ttyVK support to allow console
+ access to VK cards from host.
+
+ Device node will in the form /dev/bcm-vk.x_ttyVKy where:
+ x is the instance of the VK card
+ y is the tty device number on the VK card.
+
+endif # BCM_VK
diff --git a/drivers/misc/bcm-vk/Makefile b/drivers/misc/bcm-vk/Makefile
index e4a1486f7209..1df2ebe851ca 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_BCM_VK_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..a1338f375589 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_BCM_VK_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,13 @@ struct bcm_vk {
struct miscdevice miscdev;
int devid; /* dev id allocated */
+#ifdef CONFIG_BCM_VK_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;
+#endif
/* Reference-counting to handle file operations */
struct kref kref;
@@ -501,13 +507,43 @@ 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_BCM_VK_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);
+
+static inline void bcm_vk_tty_set_irq_enabled(struct bcm_vk *vk, int index)
+{
+ vk->tty[index].irq_enabled = true;
+}
+#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)
+{
+}
+
+static inline void bcm_vk_tty_set_irq_enabled(struct bcm_vk *vk, int index)
+{
+}
+#endif /* CONFIG_BCM_VK_TTY */
+
#endif
diff --git a/drivers/misc/bcm-vk/bcm_vk_dev.c b/drivers/misc/bcm-vk/bcm_vk_dev.c
index c3d2bba68ef1..59b4859d68d2 100644
--- a/drivers/misc/bcm-vk/bcm_vk_dev.c
+++ b/drivers/misc/bcm-vk/bcm_vk_dev.c
@@ -1396,7 +1396,7 @@ static int bcm_vk_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
pdev->irq + vk->num_irqs, vk->num_irqs + 1);
goto err_irq;
}
- vk->tty[i].irq_enabled = true;
+ bcm_vk_tty_set_irq_enabled(vk, i);
}
id = ida_simple_get(&bcm_vk_ida, 0, 0, GFP_KERNEL);
@@ -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
On 1/31/21 3:30 PM, Scott Branden wrote:
> Correct compile issue if CONFIG_TTY is not set by
> only adding ttyVK devices if CONFIG_BCM_VK_TTY is set.
>
> Reported-by: Randy Dunlap <[email protected]>
> Signed-off-by: Scott Branden <[email protected]>
>
> ---
> Changes since v2:
> - add CONFIG_BCM_VK_TTY
> - add function and stub for bcm_vk_tty_set_irq_enabled
> Changes since v1:
> - add function stubs rather than compiling out code
> ---
> drivers/misc/bcm-vk/Kconfig | 16 ++++++++++++
> drivers/misc/bcm-vk/Makefile | 4 +--
> drivers/misc/bcm-vk/bcm_vk.h | 42 +++++++++++++++++++++++++++++---
> drivers/misc/bcm-vk/bcm_vk_dev.c | 5 ++--
> drivers/misc/bcm-vk/bcm_vk_tty.c | 6 +++++
> 5 files changed, 65 insertions(+), 8 deletions(-)
Acked-by: Randy Dunlap <[email protected]> # build-tested
Thanks.
--
~Randy
On Sun, Jan 31, 2021 at 03:30:49PM -0800, Scott Branden wrote:
> Correct compile issue if CONFIG_TTY is not set by
> only adding ttyVK devices if CONFIG_BCM_VK_TTY is set.
>
> Reported-by: Randy Dunlap <[email protected]>
> Signed-off-by: Scott Branden <[email protected]>
>
> ---
> Changes since v2:
> - add CONFIG_BCM_VK_TTY
> - add function and stub for bcm_vk_tty_set_irq_enabled
> Changes since v1:
> - add function stubs rather than compiling out code
> ---
> drivers/misc/bcm-vk/Kconfig | 16 ++++++++++++
> drivers/misc/bcm-vk/Makefile | 4 +--
> drivers/misc/bcm-vk/bcm_vk.h | 42 +++++++++++++++++++++++++++++---
> drivers/misc/bcm-vk/bcm_vk_dev.c | 5 ++--
> drivers/misc/bcm-vk/bcm_vk_tty.c | 6 +++++
> 5 files changed, 65 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/misc/bcm-vk/Kconfig b/drivers/misc/bcm-vk/Kconfig
> index 052f6f28b540..16ce98c964b8 100644
> --- a/drivers/misc/bcm-vk/Kconfig
> +++ b/drivers/misc/bcm-vk/Kconfig
> @@ -15,3 +15,19 @@ config BCM_VK
> accelerators via /dev/bcm-vk.N devices.
>
> If unsure, say N.
> +
> +if BCM_VK
No need for this, just put it on the depends line, right?
> +
> +config BCM_VK_TTY
> + bool "Enable ttyVK"
Better config help text to explain what this is?
> + depends on TTY
> + default y
Default y is only there if your system can not boot without it, please
remove it.
> + help
> + Select this option to enable ttyVK support to allow console
> + access to VK cards from host.
Again, more help text, what is a "VK"?
thanks,
greg k-h
Hi Greg,,
I need a few clarifications before sending (hopefully) final revisions to the patch.
On 2021-01-31 11:45 p.m., Greg Kroah-Hartman wrote:
> On Sun, Jan 31, 2021 at 03:30:49PM -0800, Scott Branden wrote:
>> Correct compile issue if CONFIG_TTY is not set by
>> only adding ttyVK devices if CONFIG_BCM_VK_TTY is set.
>>
>> Reported-by: Randy Dunlap <[email protected]>
>> Signed-off-by: Scott Branden <[email protected]>
>>
>> ---
>> Changes since v2:
>> - add CONFIG_BCM_VK_TTY
>> - add function and stub for bcm_vk_tty_set_irq_enabled
>> Changes since v1:
>> - add function stubs rather than compiling out code
>> ---
>> drivers/misc/bcm-vk/Kconfig | 16 ++++++++++++
>> drivers/misc/bcm-vk/Makefile | 4 +--
>> drivers/misc/bcm-vk/bcm_vk.h | 42 +++++++++++++++++++++++++++++---
>> drivers/misc/bcm-vk/bcm_vk_dev.c | 5 ++--
>> drivers/misc/bcm-vk/bcm_vk_tty.c | 6 +++++
>> 5 files changed, 65 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/misc/bcm-vk/Kconfig b/drivers/misc/bcm-vk/Kconfig
>> index 052f6f28b540..16ce98c964b8 100644
>> --- a/drivers/misc/bcm-vk/Kconfig
>> +++ b/drivers/misc/bcm-vk/Kconfig
>> @@ -15,3 +15,19 @@ config BCM_VK
>> accelerators via /dev/bcm-vk.N devices.
>>
>> If unsure, say N.
>> +
>> +if BCM_VK
> No need for this, just put it on the depends line, right?
If you prefer I can but it on the depends on line.
But, I actually prefer the if syntax in this case as it more clearly shows
BCM_VK_TTY is a suboption of BCM_VK.
Please let me know which method is "right"?
>
>> +
>> +config BCM_VK_TTY
>> + bool "Enable ttyVK"
> Better config help text to explain what this is?
I'll change it to the following?
"Enable tty's on VK devices"
>
>> + depends on TTY
>> + default y
> Default y is only there if your system can not boot without it, please
> remove it.
I can remove if really needed but I'd like to learn more about such convention.
Is there a document I can learn from describing such?
We actually want a full featured driver by default. Otherwise we'll end up asking people to enable this
feature and recompile the driver to get missing features such as this.
>
>> + help
>> + Select this option to enable ttyVK support to allow console
>> + access to VK cards from host.
> Again, more help text, what is a "VK"?
VK is already described in BCM_VK. Why would I need to add the same information again to a suboption?
Perhaps you would like "config BCM_VK" changed to "menuconfig BCM_VK"
>
> thanks,
>
> greg k-h
Thanks,
Scott
On Mon, Feb 01, 2021 at 10:13:55AM -0800, Scott Branden wrote:
> Hi Greg,,
>
> I need a few clarifications before sending (hopefully) final revisions to the patch.
>
> On 2021-01-31 11:45 p.m., Greg Kroah-Hartman wrote:
> > On Sun, Jan 31, 2021 at 03:30:49PM -0800, Scott Branden wrote:
> >> Correct compile issue if CONFIG_TTY is not set by
> >> only adding ttyVK devices if CONFIG_BCM_VK_TTY is set.
> >>
> >> Reported-by: Randy Dunlap <[email protected]>
> >> Signed-off-by: Scott Branden <[email protected]>
> >>
> >> ---
> >> Changes since v2:
> >> - add CONFIG_BCM_VK_TTY
> >> - add function and stub for bcm_vk_tty_set_irq_enabled
> >> Changes since v1:
> >> - add function stubs rather than compiling out code
> >> ---
> >> drivers/misc/bcm-vk/Kconfig | 16 ++++++++++++
> >> drivers/misc/bcm-vk/Makefile | 4 +--
> >> drivers/misc/bcm-vk/bcm_vk.h | 42 +++++++++++++++++++++++++++++---
> >> drivers/misc/bcm-vk/bcm_vk_dev.c | 5 ++--
> >> drivers/misc/bcm-vk/bcm_vk_tty.c | 6 +++++
> >> 5 files changed, 65 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/misc/bcm-vk/Kconfig b/drivers/misc/bcm-vk/Kconfig
> >> index 052f6f28b540..16ce98c964b8 100644
> >> --- a/drivers/misc/bcm-vk/Kconfig
> >> +++ b/drivers/misc/bcm-vk/Kconfig
> >> @@ -15,3 +15,19 @@ config BCM_VK
> >> accelerators via /dev/bcm-vk.N devices.
> >>
> >> If unsure, say N.
> >> +
> >> +if BCM_VK
> > No need for this, just put it on the depends line, right?
> If you prefer I can but it on the depends on line.
If you do, it will help in that the default value might be inherited
from there. I think, maybe, can't remember, try it out!
> But, I actually prefer the if syntax in this case as it more clearly shows
> BCM_VK_TTY is a suboption of BCM_VK.
>
> Please let me know which method is "right"?
Either is fine, depends seems like the cleaner thing.
> >> +
> >> +config BCM_VK_TTY
> >> + bool "Enable ttyVK"
> > Better config help text to explain what this is?
> I'll change it to the following?
>
> "Enable tty's on VK devices"
"Enable tty ports on a Broadcom VK Accelerator device" is better, right?
> >> + depends on TTY
> >> + default y
> > Default y is only there if your system can not boot without it, please
> > remove it.
> I can remove if really needed but I'd like to learn more about such convention.
> Is there a document I can learn from describing such?
Again, if your machine can not boot without it, it should be 'y',
otherwise the default is n and don't list it.
> We actually want a full featured driver by default.? Otherwise we'll end up asking people to enable this
> feature and recompile the driver to get missing features such as this.
No one does that, they all use distro kernels who will enable this on
their own.
And even if they do, if they built their own kernel with your driver,
and enabled that option, then they can enable this one, there's nothing
"special" about your tiny driver here compared to the many thousands of
other ones in the tree :)
> >> + help
> >> + Select this option to enable ttyVK support to allow console
> >> + access to VK cards from host.
> > Again, more help text, what is a "VK"?
> VK is already described in BCM_VK.? Why would I need to add the same information again to a suboption?
Context is everything, why not be verbose so that people know what is
happening.
> Perhaps you would like "config BCM_VK" changed to "menuconfig BCM_VK"
No, that's overkill for a single sub-option, right?
thanks,
greg k-h