Hello,
as said before, I can only nitpick on the implementation details, hope
this is still useful :)
On Sun, Jul 23, 2023 at 6:36 AM Damien Zammit <dam...@zamaudio.com> wrote:
> +
> +/*
> + * Request io ports for a specific io region for a device.
> + * The resulting port may be used to call mach rpc i386_io_perm_modify.
> + * Only works on io region files.
> + */
> +routine pci_request_io_ports(
> + master: pci_t;
> + out io_perm: mach_port_t
> +);
io_perm: io_perm_t rather?
> +error_t
> +request_io_ports (struct pcifs_dirent *e, io_perm_t *io_perm)
> +{
> + int bar;
> + uint64_t iobase, iosize;
> + io_port_t from, to;
> + error_t err;
> + mach_port_t device_master;
> +
> + if (get_privileged_ports (0, &device_master))
> + return EPERM;
Perhaps try to propagate the error get_privileged_ports () returns?
> +
> + bar = strtol (&e->name[strlen (FILE_IO_NAME)], NULL, 10);
> + if (errno)
> + return errno;
> +
> + if ((bar < 0) || (bar > 5))
> + /* This operation only works on IO bars between 0-5 */
> + return EINVAL;
> +
> + iobase = e->device->regions[bar].base_addr;
> + iosize = e->device->regions[bar].size;
> + if ((iobase >= 0x10000) || (iosize >= 0x10000) || (iobase + iosize >=
> 0x10000))
> + return EINVAL;
In all of these early return cases, you're leaking the device_master
reference that get_privileged_ports () returns. You should just do
this validation first & get the port after them.
> +
> + from = iobase;
> + to = from + iosize - 1;
> +
> + if ((err = i386_io_perm_create (device_master, from, to, io_perm)))
> + return err;
Same here, if this fails you need to deallocate the device master. You
need to deallocate it on the success path too, so it's easiest to do:
err = i386_io_perm_create (device_master, from, to, io_perm);
if (!err)
UPDATE_TIMES (e, TOUCH_ATIME);
mach_port_deallocate (mach_task_self (), device_master);
return err;
Yes, this doesn't check the return value of mach_port_deallocate ().
It's awkward to, unless you want to introduce err2, so for cases like
this (where you care about the err value) it's OK to skip checking.
> +
> + /* Update atime */
> + UPDATE_TIMES (e, TOUCH_ATIME);
> +
> + return 0;
> +}
> +
> +error_t
> +S_pci_request_io_ports (struct protid * master, io_perm_t *io_perm)
struct protid *master
and actually I see there's quite a lot of this style (type * name) in
pci-arbiter; is that intended?
Sergey
