On Sat, Jan 29, 2011 at 09:10:00PM +0100, Juergen Lock wrote:
> Hi!
>
> I was kinda hoping to be able to post a correct patch in public but
> getting an answer to ${Subject} seems to be more difficult than I
> thought... :) So, does anyone here know? copyout_map() and
You do not need Giant locked for vm_map* functions.> copyout_unmap() are copied from ksyms_map() from sys/dev/ksyms/ksyms.c > - should there maybe be global versions instead of two static copies > each, and what would be good names? And giant is taken by linux_ioctl() Would you make a patch for this ? > in the same source file before calling the parts I added. So here > comes the patch, it is to add support for dvb ioctls to the linuxolator > as discussed on -emulation earlier in this thread: > > > http://lists.freebsd.org/pipermail/freebsd-multimedia/2011-January/011575.html > > (patch also at: > > http://people.freebsd.org/~nox/dvb/linux-dvb.patch > > and a version for 8, which is what I tested with w_scan on dvb-s2 > and dvb-t, and Andrew Gallatin also tested it with SageTV: > > http://people.freebsd.org/~nox/dvb/linux-dvb-8.patch > > ) > > Thanx! > Juergen > > Index: src/sys/compat/linux/linux_ioctl.c > =================================================================== > RCS file: /home/scvs/src/sys/compat/linux/linux_ioctl.c,v > retrieving revision 1.167 > diff -u -p -r1.167 linux_ioctl.c > --- src/sys/compat/linux/linux_ioctl.c 30 Dec 2010 02:18:04 -0000 > 1.167 > +++ src/sys/compat/linux/linux_ioctl.c 18 Jan 2011 17:10:21 -0000 > @@ -59,6 +59,14 @@ __FBSDID("$FreeBSD: src/sys/compat/linux > #include <sys/sx.h> > #include <sys/tty.h> > #include <sys/uio.h> > +#include <sys/types.h> > +#include <sys/mman.h> > +#include <sys/resourcevar.h> > + > +#include <vm/vm.h> > +#include <vm/pmap.h> > +#include <vm/vm_extern.h> > +#include <vm/vm_map.h> > > #include <net/if.h> > #include <net/if_dl.h> > @@ -83,6 +91,9 @@ __FBSDID("$FreeBSD: src/sys/compat/linux > #include <compat/linux/linux_videodev.h> > #include <compat/linux/linux_videodev_compat.h> > > +#include <compat/linux/linux_dvb.h> > +#include <compat/linux/linux_dvb_compat.h> > + > CTASSERT(LINUX_IFNAMSIZ == IFNAMSIZ); > > static linux_ioctl_function_t linux_ioctl_cdrom; > @@ -97,6 +108,7 @@ static linux_ioctl_function_t linux_ioct > static linux_ioctl_function_t linux_ioctl_drm; > static linux_ioctl_function_t linux_ioctl_sg; > static linux_ioctl_function_t linux_ioctl_v4l; > +static linux_ioctl_function_t linux_ioctl_dvb; > static linux_ioctl_function_t linux_ioctl_special; > static linux_ioctl_function_t linux_ioctl_fbsd_usb; > > @@ -124,6 +136,8 @@ static struct linux_ioctl_handler sg_han > { linux_ioctl_sg, LINUX_IOCTL_SG_MIN, LINUX_IOCTL_SG_MAX }; > static struct linux_ioctl_handler video_handler = > { linux_ioctl_v4l, LINUX_IOCTL_VIDEO_MIN, LINUX_IOCTL_VIDEO_MAX }; > +static struct linux_ioctl_handler dvb_handler = > +{ linux_ioctl_dvb, LINUX_IOCTL_DVB_MIN, LINUX_IOCTL_DVB_MAX }; > static struct linux_ioctl_handler fbsd_usb = > { linux_ioctl_fbsd_usb, FBSD_LUSB_MIN, FBSD_LUSB_MAX }; > > @@ -139,6 +153,7 @@ DATA_SET(linux_ioctl_handler_set, privat > DATA_SET(linux_ioctl_handler_set, drm_handler); > DATA_SET(linux_ioctl_handler_set, sg_handler); > DATA_SET(linux_ioctl_handler_set, video_handler); > +DATA_SET(linux_ioctl_handler_set, dvb_handler); > DATA_SET(linux_ioctl_handler_set, fbsd_usb); > > struct handler_element > @@ -2989,6 +3004,251 @@ linux_ioctl_special(struct thread *td, s > } > > /* > + * Map some anonymous memory in user space of size sz, rounded up to the page > + * boundary. > + */ > +static int > +copyout_map(struct thread *td, vm_offset_t *addr, size_t sz) > +{ > + struct vmspace *vms = td->td_proc->p_vmspace; Style recommends not to initialize in the declaration section. [I do not repeat systematic style violations notes below]. > + int error; > + vm_size_t size; > + > + One empty line is enough. > + /* > + * Map somewhere after heap in process memory. > + */ > + PROC_LOCK(td->td_proc); > + *addr = round_page((vm_offset_t)vms->vm_daddr + > + lim_max(td->td_proc, RLIMIT_DATA)); > + PROC_UNLOCK(td->td_proc); Are you sure that this is needed ? Why not leave the address selection to the VM ? > + > + /* round size up to page boundry */ > + size = (vm_size_t) round_page(sz); > + > + error = vm_mmap(&vms->vm_map, addr, size, PROT_READ | PROT_WRITE, > + VM_PROT_ALL, MAP_PRIVATE | MAP_ANON, OBJT_DEFAULT, NULL, 0); > + > + return (error); > +} > + > +/* > + * Unmap memory in user space. > + */ > +static int > +copyout_unmap(struct thread *td, vm_offset_t addr, size_t sz) > +{ > + vm_map_t map; > + vm_size_t size; > + > + map = &td->td_proc->p_vmspace->vm_map; > + size = (vm_size_t) round_page(sz); > + > + if (!vm_map_remove(map, addr, addr + size)) Better do if (vm_map_remove(...) != KERN_SUCCESS) > + return (EINVAL); > + > + return (0); > +} > + > +static int > +linux_to_bsd_dtv_properties(struct l_dtv_properties *lvps, struct > dtv_properties *vps) > +{ Empty line between (empty) declaration section and executable statements. > + vps->num = lvps->num; > + vps->props = PTRIN(lvps->props); /* possible pointer size > conversion */ > + return (0); > +} > + > +static int > +linux_to_bsd_dtv_property(struct l_dtv_property *lvp, struct dtv_property > *vp) > +{ > + /* > + * everything until u.buffer.reserved2 is fixed size so > + * just memcpy it. > + */ > + memcpy(vp, lvp, offsetof(struct l_dtv_property, u.buffer.reserved2)); > + /* > + * the pointer may be garbage since it's part of a union, > + * currently no Linux code uses it so just set it to NULL. > + */ > + vp->u.buffer.reserved2 = NULL; > + vp->result = lvp->result; > + return (0); > +} > + > +static int > +bsd_to_linux_dtv_property(struct dtv_property *vp, struct l_dtv_property > *lvp) > +{ > + /* > + * everything until u.buffer.reserved2 is fixed size so > + * just memcpy it. > + */ > + memcpy(lvp, vp, offsetof(struct l_dtv_property, u.buffer.reserved2)); > + /* > + * the pointer may be garbage since it's part of a union, > + * currently no Linux code uses it so just set it to NULL. > + */ > + lvp->u.buffer.reserved2 = PTROUT(NULL); > + lvp->result = vp->result; > + return (0); > +} > + > +static int > +linux_ioctl_dvb(struct thread *td, struct linux_ioctl_args *args) > +{ > + struct file *fp; > + int error, i; > + struct l_dtv_properties l_vps; > + struct dtv_properties vps; > + struct l_dtv_property *l_vp = NULL, *l_p; > + struct dtv_property *vp = NULL, *p; > + size_t l_propsiz, propsiz; > + vm_offset_t uvp; > + > + switch (args->cmd & 0xffff) { > + case LINUX_AUDIO_STOP: > + case LINUX_AUDIO_PLAY: > + case LINUX_AUDIO_PAUSE: > + case LINUX_AUDIO_CONTINUE: > + case LINUX_AUDIO_SELECT_SOURCE: > + case LINUX_AUDIO_SET_MUTE: > + case LINUX_AUDIO_SET_AV_SYNC: > + case LINUX_AUDIO_SET_BYPASS_MODE: > + case LINUX_AUDIO_CHANNEL_SELECT: > + case LINUX_AUDIO_CLEAR_BUFFER: > + case LINUX_AUDIO_SET_ID: > + case LINUX_AUDIO_SET_STREAMTYPE: > + case LINUX_AUDIO_SET_EXT_ID: > + case LINUX_AUDIO_BILINGUAL_CHANNEL_SELECT: > + case LINUX_DMX_START: > + case LINUX_DMX_STOP: > + case LINUX_DMX_SET_BUFFER_SIZE: > + case LINUX_NET_REMOVE_IF: > + case LINUX_FE_DISEQC_RESET_OVERLOAD: > + case LINUX_FE_DISEQC_SEND_BURST: > + case LINUX_FE_SET_TONE: > + case LINUX_FE_SET_VOLTAGE: > + case LINUX_FE_ENABLE_HIGH_LNB_VOLTAGE: > + case LINUX_FE_DISHNETWORK_SEND_LEGACY_CMD: > + case LINUX_FE_SET_FRONTEND_TUNE_MODE: > + case LINUX_CA_RESET: > + if ((args->cmd & IOC_DIRMASK) != LINUX_IOC_VOID) > + return ENOIOCTL; > + args->cmd = (args->cmd & 0xffff) | IOC_VOID; > + break; > + > + case LINUX_DMX_REMOVE_PID: > + /* overlaps with LINUX_NET_ADD_IF */ > + if ((args->cmd & IOC_DIRMASK) == LINUX_IOC_INOUT) > + goto net_add_if; > + /* FALLTHRU */ > + case LINUX_AUDIO_SET_MIXER: > + case LINUX_AUDIO_SET_ATTRIBUTES: > + case LINUX_AUDIO_SET_KARAOKE: > + case LINUX_DMX_SET_FILTER: > + case LINUX_DMX_SET_PES_FILTER: > + case LINUX_DMX_SET_SOURCE: > + case LINUX_DMX_ADD_PID: > + case LINUX_FE_DISEQC_SEND_MASTER_CMD: > + case LINUX_FE_SET_FRONTEND: > + case LINUX_CA_SEND_MSG: > + case LINUX_CA_SET_DESCR: > + case LINUX_CA_SET_PID: > + args->cmd = (args->cmd & ~IOC_DIRMASK) | IOC_IN; > + break; > + > + case LINUX_AUDIO_GET_STATUS: > + case LINUX_AUDIO_GET_CAPABILITIES: > + case LINUX_AUDIO_GET_PTS: > + case LINUX_DMX_GET_PES_PIDS: > + case LINUX_DMX_GET_CAPS: > + case LINUX_FE_GET_INFO: > + case LINUX_FE_DISEQC_RECV_SLAVE_REPLY: > + case LINUX_FE_READ_STATUS: > + case LINUX_FE_READ_BER: > + case LINUX_FE_READ_SIGNAL_STRENGTH: > + case LINUX_FE_READ_SNR: > + case LINUX_FE_READ_UNCORRECTED_BLOCKS: > + case LINUX_FE_GET_FRONTEND: > + case LINUX_FE_GET_EVENT: > + case LINUX_CA_GET_CAP: > + case LINUX_CA_GET_SLOT_INFO: > + case LINUX_CA_GET_DESCR_INFO: > + case LINUX_CA_GET_MSG: > + args->cmd = (args->cmd & ~IOC_DIRMASK) | IOC_OUT; > + break; > + > + case LINUX_DMX_GET_STC: > + case LINUX_NET_GET_IF: > + net_add_if: > + args->cmd = (args->cmd & ~IOC_DIRMASK) | IOC_INOUT; > + break; > + > + case LINUX_FE_SET_PROPERTY: > + case LINUX_FE_GET_PROPERTY: > + error = copyin((void *) args->arg, &l_vps, sizeof(l_vps)); > + if (error) > + return (error); > + linux_to_bsd_dtv_properties(&l_vps, &vps); > + if ((vps.num == 0) || vps.num > DTV_IOCTL_MAX_MSGS) > + return EINVAL; > + > + l_propsiz = vps.num * sizeof(*l_vp); > + propsiz = vps.num * sizeof(*vp); > + if (((l_vp = malloc(l_propsiz, M_LINUX, M_WAITOK)) == NULL) || > + (vp = malloc(propsiz, M_LINUX, M_WAITOK)) == NULL) { Malloc(M_WAITOK) is guaranteed to never return NULL, you do not need the checks. > + error = ENOMEM; > + goto out2; > + } > + error = copyin((void *) vps.props, l_vp, l_propsiz); > + if (error) > + goto out2; > + for (i = vps.num, l_p = l_vp, p = vp; i--; ++l_p, ++p) > + linux_to_bsd_dtv_property(l_p, p); > + > + error = copyout_map(td, &uvp, propsiz); > + if (error) > + goto out2; > + copyout(vp, (void *) uvp, propsiz); > + > + if ((error = fget(td, args->fd, &fp)) != 0) { > + (void) copyout_unmap(td, uvp, propsiz); > + goto out2; > + } > + vps.props = (void *) uvp; > + if ((args->cmd & 0xffff) == LINUX_FE_SET_PROPERTY) > + error = fo_ioctl(fp, FE_SET_PROPERTY, &vps, > td->td_ucred, td); > + else > + error = fo_ioctl(fp, FE_GET_PROPERTY, &vps, > td->td_ucred, td); > + if (error) { > + (void) copyout_unmap(td, uvp, propsiz); > + goto out; > + } > + error = copyin((void *) uvp, vp, propsiz); > + (void) copyout_unmap(td, uvp, propsiz); No need in space between cast and expression. Bigger issue is that I do not understand this fragment. You do copyout_map(), and then immediately call copyout_unmap() for the address range returned by copyout_map(), or am I missing something ? [snip]
pgpbxpAy7dSR4.pgp
Description: PGP signature
