Date: Wed, 20 Apr 2016 14:36:16 -0600
From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
To: Dennis Dalessandro <dennis.dalessandro@intel.com>
Cc: dledford@redhat.com, linux-rdma@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user
 access
Message-ID: <20160420203616.GA28991@obsidianresearch.com>
References: <20160414153727.6387.96381.stgit@scvm10.sc.intel.com>
 <20160414164550.GC6247@obsidianresearch.com>
 <20160414175243.GA9310@phlsvsds.ph.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20160414175243.GA9310@phlsvsds.ph.intel.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1587
Lines: 41

On Thu, Apr 14, 2016 at 01:52:44PM -0400, Dennis Dalessandro wrote:
> The eprom device is for low level programming of the eprom on the
> chip. We do not use i2c for this because the eprom is directly
> attached to the chip and not accessible via i2c, requires register
> access.

Okay, but the twsi.c is still not acceptable in a driver, use the i2c
subsystem to talk to the qsfps. Open coding i2c is not OK.

The char dev(s) are also done wrong, there is no set to 'kobj.parent'
and there are empty release functions. There are certainly
use-after-free bugs between close and device removal, someone needs to
audit all of this carefully.

The patch forgets compat_ioctl.

Stuff like this should be a build bug:
 /* NOTE: assumes unsigned long is 8 bytes */

The 'goto on success' in hfi1_cdev_init is an anti-pattern, don't do it.

Even if the char dev stays, creating two whole sysfs classes seems
really unnecessary. Surely there is a better place to attach the cdev.

And this is just outrageous:

        if (atomic_inc_return(&user_count) == 1) {
                ret = hfi1_cdev_init(0, class_name(), &hfi1_file_ops,
                                     &wildcard_cdev, &wildcard_device,
                                     true);
                if (ret)

I can see an argument for a per-device char dev (as Christoph says,
that is not entirely uncommon) but this is a multi-device char dev????

It is not OK to create your own private subsystem in a driver! I count
*three* char devs before this series!?!?! One of them marked 0666 even!

I stopped looking at the code.

Jason