Re: [Xenomai] Hilscher driver for cifX boards

[Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org>]

On 03/06/2013 02:58 PM, Jerome Poncin wrote:

> Gilles,
> 
> I don't know, perhaps I missed some remarks, it's possible (by error or 
> by misunderstanding) .


Again, that is because you seem to be used to top-posting, which is the opposite way this mailing list (and many others) work.

> +static ssize_t cifx_pci_write(struct rtdm_dev_context *context,
> +			      rtdm_user_info_t *user_info, const void *buf,
> +			      size_t nbyte)
> +{
> +	struct io_map_mem map_mem;
> +	int ret;
> +
> +	switch (nbyte) {
> +	case sizeof(map_mem):
> +		/* Copy data information for Kernel */
> +		if (rtdm_safe_copy_from_user(user_info, &map_mem, buf, nbyte))
> +			return 0;
> +
> +		if (*map_mem.virt_addr == NULL) {
> +			/* Map physical on virtual memory */
> +			ret = rtdm_iomap_to_user(user_info,
> +						map_mem.
> +						phys_addr,
> +						map_mem.length,
> +						(PROT_READ | PROT_WRITE),
> +						map_mem.virt_addr,
> +						NULL,
> +						NULL);
> +
> +			if (ret != 0)
> +				return 0;
> +		} else {
> +			/* Unap virtual memory */
> +			ret = rtdm_munmap(user_info,
> +					*map_mem.virt_addr,
> +					map_mem.length);
> +
> +			if (ret != 0)
> +				return 0;
> +		}
> +		break;
> +
> +	default:
> +		/* Error */
> +		return 0;
> +	}
> +
> +	return nbyte;


Again, left alone the fact that this function should not be a "write" 
method, this should be:

if (nbyte != sizeof(mmap_mem))
	return -EINVAL;

if (rtdm_safe_copy_from_user(user_info, &map_mem, buf, nbyte))
	return -EFAULT;

if (*map_mem.virt_addr)
	return rtdm_munmap(user_info, *map_mem.virt_addr, map_mem.length);

return rtdm_iomap_to_user(user_info, map_mem.phys_addr, map_mem.length,
			(PROT_READ | PROT_WRITE), map_mem.virt_addr, NULL, NULL);


And I am wondering, if this is even correct: if map_mem.virt_addr is a 
user-space address, it is forbidden to dereference it directly in 
kernel-space, we should use another copy_from/to_user, even if it appears
to work on some (most in fact) architectures. It is probably better to 
put the virtual address directly in the map_mem structure instead of
a pointer requiring more contorsions.

-- 
                                                                Gilles.