[PATCH 1/3] um: Monitor error events in IRQ controller

[PATCH 1/3] um: Monitor error events in IRQ controller

[anton.ivanov]

From: Anton Ivanov <anton.ivanov@cambridgegreys.com>

Ensure that file closes, connection closes, etc are propagated
as interrupts in the interrupt controller.

Fixes: ff6a17989c08 ("Epoll based IRQ controller")
Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
---
 arch/um/os-Linux/irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/um/os-Linux/irq.c b/arch/um/os-Linux/irq.c
index d508310ee5e1..f1732c308c61 100644
--- a/arch/um/os-Linux/irq.c
+++ b/arch/um/os-Linux/irq.c
@@ -48,7 +48,7 @@ int os_epoll_triggered(int index, int events)
 int os_event_mask(int irq_type)
 {
 	if (irq_type == IRQ_READ)
-		return EPOLLIN | EPOLLPRI;
+		return EPOLLIN | EPOLLPRI | EPOLLERR | EPOLLHUP | EPOLLRDHUP;
 	if (irq_type == IRQ_WRITE)
 		return EPOLLOUT;
 	return 0;
-- 
2.20.1


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

[PATCH 2/3] um: tty: Fix handling of close in tty lines

[anton.ivanov]

From: Anton Ivanov <anton.ivanov@cambridgegreys.com>

Fix a logical error in tty reading. We get 0 and errno == EAGAIN
on the first attempt to read from a closed file descriptor.

Compared to that a true EAGAIN is EAGAIN and -1.

If we check errno for EAGAIN first, before checking the return
value we miss the fact that the descriptor is closed.

This bug is as old as the driver. It was not showing up with
the original POLL based IRQ controller, because it was
producing multiple events. Switching to EPOLL unmasked it.

Fixes: ff6a17989c08 ("Epoll based IRQ controller")
Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
---
 arch/um/drivers/chan_user.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/um/drivers/chan_user.c b/arch/um/drivers/chan_user.c
index 4d80526a4236..d8845d4aac6a 100644
--- a/arch/um/drivers/chan_user.c
+++ b/arch/um/drivers/chan_user.c
@@ -26,10 +26,10 @@ int generic_read(int fd, char *c_out, void *unused)
 	n = read(fd, c_out, sizeof(*c_out));
 	if (n > 0)
 		return n;
-	else if (errno == EAGAIN)
-		return 0;
 	else if (n == 0)
 		return -EIO;
+	else if (errno == EAGAIN)
+		return 0;
 	return -errno;
 }
 
-- 
2.20.1


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

[PATCH 3/3] um: chan_xterm: Fix fd leak

[anton.ivanov]

From: Anton Ivanov <anton.ivanov@cambridgegreys.com>

xterm serial channel was leaking a fd used in setting up the
port helper

This bug is prehistoric - it predates switching to git. The "fixes"
header here is really just to mark all the versions we would like this to
apply to which is "Anything from the Cretaceous period onwards".

No dinosaurs were harmed in fixing this bug.

Fixes: b40997b872cd ("um: drivers/xterm.c: fix a file descriptor leak")
Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
---
 arch/um/drivers/xterm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/um/drivers/xterm.c b/arch/um/drivers/xterm.c
index fc7f1e746703..87ca4a47cd66 100644
--- a/arch/um/drivers/xterm.c
+++ b/arch/um/drivers/xterm.c
@@ -18,6 +18,7 @@
 struct xterm_chan {
 	int pid;
 	int helper_pid;
+	int chan_fd;
 	char *title;
 	int device;
 	int raw;
@@ -33,6 +34,7 @@ static void *xterm_init(char *str, int device, const struct chan_opts *opts)
 		return NULL;
 	*data = ((struct xterm_chan) { .pid 		= -1,
 				       .helper_pid 	= -1,
+				       .chan_fd		= -1,
 				       .device 		= device,
 				       .title 		= opts->xterm_title,
 				       .raw  		= opts->raw } );
@@ -149,6 +151,7 @@ static int xterm_open(int input, int output, int primary, void *d,
 		goto out_kill;
 	}
 
+	data->chan_fd = fd;
 	new = xterm_fd(fd, &data->helper_pid);
 	if (new < 0) {
 		err = new;
@@ -206,6 +209,8 @@ static void xterm_close(int fd, void *d)
 		os_kill_process(data->helper_pid, 0);
 	data->helper_pid = -1;
 
+	if (data->chan_fd != -1)
+		os_close_file(data->chan_fd);
 	os_close_file(fd);
 }
 
-- 
2.20.1


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH 1/3] um: Monitor error events in IRQ controller

[Johannes Berg]

On Mon, 2020-12-07 at 17:19 +0000, anton.ivanov@cambridgegreys.com
wrote:
> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> 
> Ensure that file closes, connection closes, etc are propagated
> as interrupts in the interrupt controller.
> 
> Fixes: ff6a17989c08 ("Epoll based IRQ controller")
> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> ---
>  arch/um/os-Linux/irq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/um/os-Linux/irq.c b/arch/um/os-Linux/irq.c
> index d508310ee5e1..f1732c308c61 100644
> --- a/arch/um/os-Linux/irq.c
> +++ b/arch/um/os-Linux/irq.c
> @@ -48,7 +48,7 @@ int os_epoll_triggered(int index, int events)
>  int os_event_mask(int irq_type)
>  {
>  	if (irq_type == IRQ_READ)
> -		return EPOLLIN | EPOLLPRI;
> +		return EPOLLIN | EPOLLPRI | EPOLLERR | EPOLLHUP | EPOLLRDHUP;
>  	if (irq_type == IRQ_WRITE)
>  		return EPOLLOUT;

Why not monitor it also for write?

johannes


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH 1/3] um: Monitor error events in IRQ controller

[Anton Ivanov]

On 07/12/2020 17:35, Johannes Berg wrote:
> On Mon, 2020-12-07 at 17:19 +0000, anton.ivanov@cambridgegreys.com
> wrote:
>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>
>> Ensure that file closes, connection closes, etc are propagated
>> as interrupts in the interrupt controller.
>>
>> Fixes: ff6a17989c08 ("Epoll based IRQ controller")
>> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>> ---
>>   arch/um/os-Linux/irq.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/um/os-Linux/irq.c b/arch/um/os-Linux/irq.c
>> index d508310ee5e1..f1732c308c61 100644
>> --- a/arch/um/os-Linux/irq.c
>> +++ b/arch/um/os-Linux/irq.c
>> @@ -48,7 +48,7 @@ int os_epoll_triggered(int index, int events)
>>   int os_event_mask(int irq_type)
>>   {
>>   	if (irq_type == IRQ_READ)
>> -		return EPOLLIN | EPOLLPRI;
>> +		return EPOLLIN | EPOLLPRI | EPOLLERR | EPOLLHUP | EPOLLRDHUP;
>>   	if (irq_type == IRQ_WRITE)
>>   		return EPOLLOUT;
> 
> Why not monitor it also for write?

The write code in most drivers has no error checking and no close upon 
error.

I will have to go through them and ensure they behave correctly first.

So - yes, it's a good idea and we will add it later, once I have 
double-checked all drivers.

> 
> johannes
> 
> 
> _______________________________________________
> linux-um mailing list
> linux-um@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-um
> 


-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH 1/3] um: Monitor error events in IRQ controller

[Anton Ivanov]

On 07/12/2020 18:11, Anton Ivanov wrote:
> On 07/12/2020 17:35, Johannes Berg wrote:
>> On Mon, 2020-12-07 at 17:19 +0000, anton.ivanov@cambridgegreys.com
>> wrote:
>>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>>
>>> Ensure that file closes, connection closes, etc are propagated
>>> as interrupts in the interrupt controller.
>>>
>>> Fixes: ff6a17989c08 ("Epoll based IRQ controller")
>>> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>> ---
>>>   arch/um/os-Linux/irq.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/um/os-Linux/irq.c b/arch/um/os-Linux/irq.c
>>> index d508310ee5e1..f1732c308c61 100644
>>> --- a/arch/um/os-Linux/irq.c
>>> +++ b/arch/um/os-Linux/irq.c
>>> @@ -48,7 +48,7 @@ int os_epoll_triggered(int index, int events)
>>>   int os_event_mask(int irq_type)
>>>   {
>>>       if (irq_type == IRQ_READ)
>>> -        return EPOLLIN | EPOLLPRI;
>>> +        return EPOLLIN | EPOLLPRI | EPOLLERR | EPOLLHUP | EPOLLRDHUP;
>>>       if (irq_type == IRQ_WRITE)
>>>           return EPOLLOUT;
>>
>> Why not monitor it also for write?
>
> The write code in most drivers has no error checking and no close upon 
> error.
>
> I will have to go through them and ensure they behave correctly first.
>
> So - yes, it's a good idea and we will add it later, once I have 
> double-checked all drivers.

I have a couple of further clean-up patches to follow, but they need 
both your series and my series to go in first.

They are mostly cosmetic - ensuring correct behavior on epoll errors and 
suppressing messages for conditions which are not really errors, but 
"business as usual".

A.

>
>>
>> johannes
>>
>>
>> _______________________________________________
>> linux-um mailing list
>> linux-um@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-um
>>
>
>

-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um