[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] term/serial: Add support for PCI serial devices
From: |
Glenn Washburn |
Subject: |
Re: [PATCH] term/serial: Add support for PCI serial devices |
Date: |
Thu, 25 Aug 2022 14:23:34 -0500 |
On Thu, 25 Aug 2022 10:10:45 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
>
>
> How's this then? seems to build. If this looks allright, I'll post it as
> v2 I suppose.
Yes, this is what I was thinking, with the nice addition of
GRUB_HAS_PCI, which I wasn't considering.
>
> ---
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 5212dfab1369..c0683da2353b 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -224,12 +224,14 @@ kernel = {
>
> i386_efi = kern/i386/efi/init.c;
> i386_efi = bus/pci.c;
> + i386_efi_cppflags = '-DGRUB_HAS_PCI';
I imagine this should be enabled for all i386 and x86_64 targets, not
just the few that you have here. For instance, what about i386-pc,
which is the 32-bit BIOS target?
>
> x86_64 = kern/x86_64/dl.c;
> x86_64_xen = kern/x86_64/dl.c;
> x86_64_efi = kern/x86_64/efi/callwrap.S;
> x86_64_efi = kern/i386/efi/init.c;
> x86_64_efi = bus/pci.c;
> + x86_64_efi_cppflags = '-DGRUB_HAS_PCI';
>
> xen = kern/i386/tsc.c;
> xen = kern/i386/xen/tsc.c;
> @@ -271,6 +273,7 @@ kernel = {
> i386_pc = term/i386/pc/console.c;
>
> i386_qemu = bus/pci.c;
> + i386_qemu_cppflags = '-DGRUB_HAS_PCI';
> i386_qemu = kern/vga_init.c;
> i386_qemu = kern/i386/qemu/mmap.c;
>
> @@ -303,6 +306,7 @@ kernel = {
> mips_loongson = bus/bonito.c;
> mips_loongson = bus/cs5536.c;
> mips_loongson = bus/pci.c;
> + mips_loongson_cppflags = '-DGRUB_HAS_PCI';
> mips_loongson = kern/mips/loongson/init.c;
> mips_loongson = term/at_keyboard.c;
> mips_loongson = term/ps2.c;
> @@ -2047,6 +2051,8 @@ module = {
> ieee1275 = term/ieee1275/serial.c;
> mips_arc = term/arc/serial.c;
> efi = term/efi/serial.c;
> + x86 = term/pci/serial.c;
> + mips_loongson = term/pci/serial.c;
>
> enable = terminfomodule;
> enable = ieee1275;
> diff --git a/grub-core/term/pci/serial.c b/grub-core/term/pci/serial.c
> new file mode 100644
> index 000000000000..581d0760f9f4
> --- /dev/null
> +++ b/grub-core/term/pci/serial.c
> @@ -0,0 +1,83 @@
> +/*
> + * GRUB -- GRand Unified Bootloader
> + * Copyright (C) 2000,2001,2002,2003,2004,2005,2007,2008,2009,2010 Free
> Software Foundation, Inc.
s/2000,2001,2002,2003,2004,2005,2007,2008,2009,2010/2022/
> + *
> + * GRUB is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * GRUB is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/serial.h>
> +#include <grub/term.h>
> +#include <grub/types.h>
> +#include <grub/pci.h>
> +#include <grub/mm.h>
> +#include <grub/misc.h>
> +
> +static int
> +find_pciserial (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
> +{
> + grub_pci_address_t cmd_addr, class_addr, bar_addr;
> + struct grub_serial_port *port;
> + grub_uint32_t class, bar;
> + grub_uint16_t cmdreg;
> + int *nr = data;
I'd prefer a more descriptive name, like port_num.
> + int err;
s/int/grub_err_t/
> +
> + (void)pciid;
> +
> + cmd_addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
> + cmdreg = grub_pci_read (cmd_addr);
> +
> + class_addr = grub_pci_make_address (dev, GRUB_PCI_REG_REVISION);
> + class = grub_pci_read (class_addr);
> +
> + bar_addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
> + bar = grub_pci_read (bar_addr);
> +
> + /* 16550 compattible MODEM or SERIAL */
> + if (((class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_MODEM &&
> + (class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_SERIAL) ||
> + ((class >> 8) & 0xff) != GRUB_PCI_SERIAL_16550_COMPATIBLE)
> + return 0;
> +
> + if ((bar & GRUB_PCI_ADDR_SPACE_MASK) != GRUB_PCI_ADDR_SPACE_IO)
> + return 0;
> +
> + port = grub_zalloc (sizeof (*port));
> + if (!port)
Daniel prefers: port != NULL
> + return 0;
> +
> + grub_pci_write (cmd_addr, cmdreg | GRUB_PCI_COMMAND_IO_ENABLED);
> +
> + port->name = grub_xasprintf ("pci%d", (*nr)++);
Incrementing nr should happen just before the return on the success
path.
I believe there will be a memory leak when removing the serial module.
And I'm noticing that there appears to be a memory leak in the efi
serial code also. So maybe this is acceptable.
There should probably be code like:
if (port->name == NULL)
goto error;
> + port->driver = &grub_ns8250_driver;
> + port->port = bar & GRUB_PCI_ADDR_IO_MASK;
> + err = grub_serial_config_defaults (port);
> + if (err)
if (err != GRUB_ERR_NONE)
> + {
> + grub_print_error ();
> + return 0;
goto error;
> + }
> +
> + grub_serial_register (port);
err = grub_serial_register (port);
if (err != GRUB_ERR_NONE)
goto error;
(*port_num)++;
> +
> + return 0;
error:
grub_free (port->name);
grub_free (port);
return 0;
> +}
> +
> +void
> +grub_pciserial_init (void)
> +{
> + int nr = 0;
Let's rename this also.
> +
> + grub_pci_iterate (find_pciserial, &nr);
> +}
> diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
> index 842322b1c607..09c9e1cd1fdc 100644
> --- a/grub-core/term/serial.c
> +++ b/grub-core/term/serial.c
> @@ -448,6 +448,9 @@ GRUB_MOD_INIT(serial)
> #ifdef GRUB_MACHINE_ARC
> grub_arcserial_init ();
> #endif
> +#ifdef GRUB_HAS_PCI
> + grub_pciserial_init ();
> +#endif
> }
>
> GRUB_MOD_FINI(serial)
> diff --git a/include/grub/pci.h b/include/grub/pci.h
> index 262c89b748bb..b228bb1b71fd 100644
> --- a/include/grub/pci.h
> +++ b/include/grub/pci.h
> @@ -80,8 +80,13 @@
>
> #define GRUB_PCI_STATUS_DEVSEL_TIMING_SHIFT 9
> #define GRUB_PCI_STATUS_DEVSEL_TIMING_MASK 0x0600
> +
> #define GRUB_PCI_CLASS_SUBCLASS_VGA 0x0300
> #define GRUB_PCI_CLASS_SUBCLASS_USB 0x0c03
> +#define GRUB_PCI_CLASS_COMMUNICATION_SERIAL 0x0700
> +#define GRUB_PCI_CLASS_COMMUNICATION_MODEM 0x0703
> +
> +#define GRUB_PCI_SERIAL_16550_COMPATIBLE 0x02
>
> #ifndef ASM_FILE
>
> diff --git a/include/grub/serial.h b/include/grub/serial.h
> index 67379de45dd1..c57aed19ff2b 100644
> --- a/include/grub/serial.h
> +++ b/include/grub/serial.h
> @@ -193,6 +193,8 @@ const char *
> grub_arcserial_add_port (const char *path);
> #endif
>
> +void grub_pciserial_init (void);
I think this should be wrapped in "#ifdef GRUB_HAS_PCI" for clarity of
use.
Glenn
> +
> struct grub_serial_port *grub_serial_find (const char *name);
> extern struct grub_serial_driver grub_ns8250_driver;
> void EXPORT_FUNC(grub_serial_unregister_driver) (struct grub_serial_driver
> *driver);
- [PATCH] term/serial: Add support for PCI serial devices, Peter Zijlstra, 2022/08/24
- Re: [PATCH] term/serial: Add support for PCI serial devices, Glenn Washburn, 2022/08/24
- Re: [PATCH] term/serial: Add support for PCI serial devices, Peter Zijlstra, 2022/08/24
- Re: [PATCH] term/serial: Add support for PCI serial devices, Glenn Washburn, 2022/08/24
- Re: [PATCH] term/serial: Add support for PCI serial devices, Peter Zijlstra, 2022/08/25
- Re: [PATCH] term/serial: Add support for PCI serial devices, Peter Zijlstra, 2022/08/25
- Re: [PATCH] term/serial: Add support for PCI serial devices, Glenn Washburn, 2022/08/25
- Re: [PATCH] term/serial: Add support for PCI serial devices, Peter Zijlstra, 2022/08/25
- Re: [PATCH] term/serial: Add support for PCI serial devices, Glenn Washburn, 2022/08/25
- Re: [PATCH] term/serial: Add support for PCI serial devices, Peter Zijlstra, 2022/08/25
- Re: [PATCH] term/serial: Add support for PCI serial devices,
Glenn Washburn <=
- Re: [PATCH] term/serial: Add support for PCI serial devices, Peter Zijlstra, 2022/08/25
- Re: [PATCH] term/serial: Add support for PCI serial devices, Glenn Washburn, 2022/08/26
Re: [PATCH] term/serial: Add support for PCI serial devices, Peter Zijlstra, 2022/08/25
Re: [PATCH] term/serial: Add support for PCI serial devices, Gerd Hoffmann, 2022/08/25