Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Date:   Mon, 27 Nov 2023 18:59:12 +0000
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Saeed Mahameed <saeed@kernel.org>
Cc:     Arnd Bergmann <arnd@arndb.de>, Jason Gunthorpe <jgg@nvidia.com>,
        Leon Romanovsky <leonro@nvidia.com>,
        Jiri Pirko <jiri@nvidia.com>, Leonid Bloch <lbloch@nvidia.com>,
        Itay Avraham <itayavr@nvidia.com>,
        Jakub Kicinski <kuba@kernel.org>, linux-kernel@vger.kernel.org,
        Saeed Mahameed <saeedm@nvidia.com>
Subject: Re: [PATCH V3 2/5] misc: mlx5ctl: Add mlx5ctl misc driver
Message-ID: <2023112727-caddie-eardrum-efe8@gregkh>
References: <20231121070619.9836-1-saeed@kernel.org>
 <20231121070619.9836-3-saeed@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231121070619.9836-3-saeed@kernel.org>
Precedence: bulk

On Mon, Nov 20, 2023 at 11:06:16PM -0800, Saeed Mahameed wrote:
> +struct mlx5ctl_dev {
> +	struct mlx5_core_dev *mdev;
> +	struct miscdevice miscdev;
> +	struct auxiliary_device *adev;
> +	struct list_head fd_list;
> +	spinlock_t fd_list_lock; /* protect list add/del */
> +	struct rw_semaphore rw_lock;
> +	struct kref refcount;

You now have 2 different things that control the lifespan of this
structure.  We really need some way to automatically check this so that
people don't keep making this same mistake, it happens all the time :(

Please pick one structure (miscdevice) or the other (kref) to control
the lifespan, having 2 will just not work.

Also, why a rw_semaphore?  Only use those if you can prove with a
benchmark that it is actually faster, otherwise it's simpler to just use
a normal mutex (hint, you are changing the fields in the structure with
the read lock held, which feels very wrong, and so needs a LOT of
documentation, or just use a normal mutex...)

thanks,

greg k-h