Re: [PATCH 06/41] IB/hfi1: add char device instantiation code

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Wed, Jun 17, 2015 at 12:05:08PM +0000, Marciniszyn, Mike wrote:

> Jason, what is your take on ioctl vs. write.

Well, I think the global view keeps changing, so I'm not sure what is
trendy now..

But personally, I hate seeing write() used to emulate ioctl() because
'ioctl is bad'. ie if you are 'writing' a struct that contains user
pointers and you expect the kernel to read/write to user memory, then
use ioctl. (and that is the 'badness' of ioctl, so pretending it is
write doesn't help anything)

If you can formulate your communication like netlink does, where it is
more like a network RPC, where *everything* is in the buffer passed
to write and you have to call read to get a reply, then use
read/write. This is generally considered preferable, and is what
people mean when they say write is preferred. As I understand
it.

netlink is a reasonable low speed format to use for this kind of
serialization, either via the common mux or via your own char device.

I also wonder about all those sysfs files. I think the over reliance
on sysfs in rdma may have been a mistake, sending the same information
over netlink would be more consistent with what netdev is doing. (eg
you can't view the netdev IP addresses via sysfs, but you can view
rdma guids via sysfs)

Overall, I'd be alot happier with your driver patchset if it was split
as 'core only, no UAPI changes' followed up by 'Add UAPI for X'.

If you can't make a verbs provider work under 'core only' then I think
something is very wrong...

UAPI stuff in drivers is often a red flag, and you guys should think
really carefully about what OPA elements should be buried in the
driver and what elements should be common to all OPA adapters.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html