Page MenuHomeFreeBSD
Differential D8128 Diff 20951 sys/dev/efidev/efidev.c

Changeset View

Standalone View

View Options

sys/dev/efidev/efidev.c

  • This file was added.
/*-
* Copyright (c) 2016 Netflix, Inc.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer
* in this position and unchanged.
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
*
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
* IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
* OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
* IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
* INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
* NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
* THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");
#include <sys/param.h>
#include <sys/systm.h>
#include <sys/kernel.h>
#include <sys/bus.h>
#include <sys/conf.h>
#include <sys/lock.h>
#include <sys/malloc.h>
#include <sys/module.h>
#include <machine/efi.h>
kibUnsubmitted
Not Done
Inline Actions

I recommend going with sys/efi.h instead of machine/efi.h. I put the access KPI definitions into machine/efi.h for now only because only amd64 was handled. If any other arch grows the KPI support, most likely the declarations would migrate to sys/, and sys/efi.h includes machine/efi.h anyway.

kib: I recommend going with sys/efi.h instead of machine/efi.h. I put the access KPI definitions…
impAuthorUnsubmitted
Not Done
Inline Actions

Agreed.

imp: Agreed.
#include <sys/efiio.h>
static d_ioctl_t efidev_ioctl;
static struct cdevsw efi_cdevsw = {
.d_name = "efi",
.d_version = D_VERSION,
.d_ioctl = efidev_ioctl,
};
/* ARGSUSED */
kibUnsubmitted
Not Done
Inline Actions

I do not think there is any use of the lint keywords in the new code. I will remove lint in near future, I hope.

kib: I do not think there is any use of the lint keywords in the new code. I will remove lint in…
impAuthorUnsubmitted
Not Done
Inline Actions

Yea, I can delete it. It was copied from something else.

imp: Yea, I can delete it. It was copied from something else.
static int
efidev_ioctl(struct cdev *dev __unused, u_long cmd, caddr_t addr,
int flags __unused, struct thread *td __unused)
{
int error;
switch (cmd) {
case EFIIOC_GET_TABLE:
{
struct efi_get_table_ioc *egtioc =
(struct efi_get_table_ioc *)addr;
error = efi_get_table(&egtioc->uuid, &egtioc->ptr);
break;
}
case EFIIOC_GET_TIME:
{
struct efi_tm *tm = (struct efi_tm *)addr;
error = efi_get_time(tm);
break;
}
case EFIIOC_SET_TIME:
{
struct efi_tm *tm = (struct efi_tm *)addr;
error = efi_set_time(tm);
break;
}
case EFIIOC_VAR_GET:
{
struct efi_var_ioc *ev = (struct efi_var_ioc *)addr;
void *data = NULL;
efi_char *name = NULL;
data = malloc(ev->datasize, M_TEMP, M_WAITOK);
if (data == NULL) {
kibUnsubmitted
Done
Inline Actions

This is impossible.

kib: This is impossible.
error = ENOMEM;
goto vg_out;
}
name = malloc(ev->namesize, M_TEMP, M_WAITOK);
if (name == NULL) {
error = ENOMEM;
goto vg_out;
}
error = copyin(ev->name, name, ev->namesize);
if (error)
goto vg_out;
if (name[ev->namesize / sizeof(efi_char)] != 0) {
error = EINVAL;
goto vg_out;
}
error = efi_var_get(name, &ev->vendor, &ev->attrib,
&ev->datasize, data);
if (error == 0) {
error = copyout(data, ev->data, ev->datasize);
} else if (error == EOVERFLOW) {
/*
* Pass back the size we really need, but
* convert the error to 0 so the copyout
* happens. datasize was updated in the
* efi_var_get call.
*/
ev->data = NULL;
error = 0;
}
vg_out:
emasteUnsubmitted
Done
Inline Actions

feels unusual to me to indent the label

emaste: feels unusual to me to indent the label
impAuthorUnsubmitted
Done
Inline Actions

It's the style I've used since the 80's...
However, looks like 99+% of rest of the kernel doesn't indent at all...
And about 1/4 of that <1% is my doing.

imp: It's the style I've used since the 80's... However, looks like 99+% of rest of the kernel…
if (data != NULL)
kibUnsubmitted
Done
Inline Actions

The checks for NULL are not needed.

kib: The checks for NULL are not needed.
impAuthorUnsubmitted
Done
Inline Actions

Derp! You're right. Habit.

imp: Derp! You're right. Habit.
free(data, M_TEMP);
if (name != NULL)
free(name, M_TEMP);
break;
}
case EFIIOC_VAR_NEXT:
{
struct efi_var_ioc *ev = (struct efi_var_ioc *)addr;
efi_char *name = NULL;
name = malloc(ev->namesize, M_TEMP, M_WAITOK);
if (name == NULL) {
error = ENOMEM;
goto vn_out;
}
error = copyin(ev->name, name, ev->namesize);
if (error)
goto vn_out;
/* Note: namesize is the buffer size, not the string lenght */
martymacUnsubmitted
Done
Inline Actions

s/lenght/length/

martymac: s/lenght/length/
error = efi_var_nextname(&ev->namesize, name, &ev->vendor);
if (error == 0) {
error = copyout(name, ev->name, ev->namesize);
} else if (error == EOVERFLOW) {
ev->name = NULL;
error = 0;
}
vn_out:
if (name != NULL)
free(name, M_TEMP);
break;
}
case EFIIOC_VAR_SET:
{
struct efi_var_ioc *ev = (struct efi_var_ioc *)addr;
void *data = NULL;
efi_char *name = NULL;
/* datasize == 0 -> delete (more or less) */
if (ev->datasize > 0) {
data = malloc(ev->datasize, M_TEMP, M_WAITOK);
if (data == NULL) {
error = ENOMEM;
goto vs_out;
}
}
name = malloc(ev->namesize, M_TEMP, M_WAITOK);
if (name == NULL) {
error = ENOMEM;
goto vs_out;
}
if (ev->datasize) {
martymacUnsubmitted
Done
Inline Actions

Same test as above, use :
if (ev->datasize > 0)
for consistency ?

martymac: Same test as above, use : if (ev->datasize > 0) for consistency ?
impAuthorUnsubmitted
Done
Inline Actions

No, it's real. You pass in datasize = 0 when you want to delete the variable.

imp: No, it's real. You pass in datasize = 0 when you want to delete the variable.
martymacUnsubmitted
Not Done
Inline Actions

Oops, right

martymac: Oops, right
error = copyin(ev->data, data, ev->datasize);
if (error)
goto vs_out;
}
error = copyin(ev->name, name, ev->namesize);
if (error)
goto vs_out;
if (name[ev->namesize / sizeof(efi_char)] != 0) {
error = EINVAL;
goto vg_out;
}
error = efi_var_set(name, &ev->vendor, ev->attrib, ev->datasize,
data);
vs_out:
if (data != NULL)
free(data, M_TEMP);
if (name != NULL)
free(name, M_TEMP);
break;
}
default:
error = ENOTTY;
break;
}
return (error);
}
struct efidev_softc
{
kibUnsubmitted
Done
Inline Actions

Opening brace should be on the previous line.

kib: Opening brace should be on the previous line.
struct cdev *cdev;
};
static void
efidev_identify(driver_t * driver, device_t parent)
{
device_t child;
child = device_find_child(parent, driver->name, 0);
if (child == NULL)
child = BUS_ADD_CHILD(parent, 0, "efidev", -1);
device_set_driver(child, driver);
}
static int
efidev_probe(device_t dev)
{
if (!efi_rt_avail())
return (ENXIO);
device_set_desc(dev, "efi runtime services");
return (0);
}
static int
efidev_attach(device_t dev)
{
struct efidev_softc *sc;
sc = device_get_softc(dev);
sc->cdev = make_dev(&efi_cdevsw, 0, UID_ROOT, GID_WHEEL, 0700,
martymacUnsubmitted
Done
Inline Actions

Is 0700 really needed ? 0600 ?

martymac: Is 0700 really needed ? 0600 ?
impAuthorUnsubmitted
Done
Inline Actions

Yea.

imp: Yea.
"efidev");
kibUnsubmitted
Done
Inline Actions

Why not /dev/efi or /dev/efirt ? /dev/efidev is arguably redundant.

kib: Why not /dev/efi or /dev/efirt ? /dev/efidev is arguably redundant.
impAuthorUnsubmitted
Done
Inline Actions

Yea, /dev/efi is better.

imp: Yea, /dev/efi is better.
return (0);
}
static int
efidev_detach(device_t dev)
{
struct efidev_softc *sc;
sc = device_get_softc(dev);
destroy_dev(sc->cdev);
return (0);
}
static device_method_t efidev_methods[] = {
kibUnsubmitted
Not Done
Inline Actions

I do not see the newbus attachment for the software-only construct useful. Do you have any other plans for the efi device_t ? IMO it is just useless code: you may fail module load if efirt unable to provide runtime access.

kib: I do not see the newbus attachment for the software-only construct useful. Do you have any…
impAuthorUnsubmitted
Not Done
Inline Actions

I'm not sure which way to go. It's useful to have something in device_t to make it more visible. On the other hand, this is currently the only device exported for EFI. It's been quite useful to have the two in separate modules so I can reload this code w/o rebooting.
I'll consider the request, but here our philosophies may differ.

imp: I'm not sure which way to go. It's useful to have something in device_t to make it more visible.
/* Device interface */
DEVMETHOD(device_identify, efidev_identify),
DEVMETHOD(device_probe, efidev_probe),
DEVMETHOD(device_attach, efidev_attach),
DEVMETHOD(device_detach, efidev_detach),
DEVMETHOD_END
};
static driver_t efidev_driver = {
"efidev",
efidev_methods,
sizeof(struct efidev_softc),
};
static devclass_t efidev_devclass;
DRIVER_MODULE(efidev, nexus, efidev_driver, efidev_devclass, NULL, NULL);
MODULE_VERSION(efidev, 1);