Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 19 Jan 2022 08:34:22 +1300
From:   Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     gregkh@linuxfoundation.org, realwakka@gmail.com,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/3] staging: pi433: validate max bit_rate based on
 modulation used
Message-ID: <20220118193422.GA3155@mail.google.com>
References: <20220117055849.GA10644@mail.google.com>
 <20220117060220.GA11202@mail.google.com>
 <20220118135902.GH1951@kadam>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220118135902.GH1951@kadam>
Precedence: bulk

On Tue, Jan 18, 2022 at 04:59:02PM +0300, Dan Carpenter wrote:
> At the same time, this is a GCC warning so it will break the build.
> 
> Instead of applying this patch, I wish you would just make a note of it
> in the drivers/staging/pi433/TODO file.  
> 
>     "Change (struct pi433_tx_cfg)->bit_rate to be a u32 so that we can
>      support bit rates up to 300kbps per the spec."

thanks for taking the time to review this patchset.

It makes sense to me. So essentially for this version of the patchset I
would only validate OOK max bit rate as it fits within the u16 boundary
of the current implementation, right?

Something akin to this:

// check input value
if (bit_rate < 1200 || (mod == OOK && bit_rate > 32768)) {
	dev_dbg(&spi->dev, "setBitRate: illegal input param");
	return -EINVAL;
}

> 
> But you're right that it's complicated to fix this because it's part of
> the UAPI.  I think that the UAPI for pi433 is kind of garbage.  No one
> like custom ioctls.  It would be better to use sysfs.  
> ...
> So my idea is that instead of modifying the custom ioctl then we can
> just add a new sysfs file to set the bit_rate and all the other stuff.
> Eventually we will delete the ioctl after all the users have updated to
> the new userspace.
> 

Using sysfs or even possibly configfs was one of the things I had in
mind too. 

I started a similar discussion on how to change/remove ioctl:
https://lore.kernel.org/kernelnewbies/YeVoAlv0ubKrmckV@kroah.com/T/#t

The tldr;:

If there is a userspace tool that interfaces with char
device and we can change it at the same time as the kernel code then we 
can have a bit more flexibility on how to send/retrieve values to/from 
the driver. If not then we would be required to keep the original ioctl
compatibility even if we move to a different approach due to the
impossibility of knowing whether or not someone is relying on that. :(

Everything leads me to believe that there isn't such userspace tool /
lib so I emailed the original author just to be 100% sure that this is
the case.

thanks,

Paulo Almeida