Date:   Mon, 10 Jan 2022 12:17:23 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Cc:     gregkh@linuxfoundation.org, realwakka@gmail.com,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: pi433: enforce tx_cfg to be set before any
 message can be sent
Message-ID: <20220110091723.GB1951@kadam>
References: <20220108222652.GA11883@mail.google.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220108222652.GA11883@mail.google.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?4XIQXQ4Dm3rk2TjPIozquDOCoIxNE2T/1zs+yXPOWW7PoLblIO1TH7N/F1PV?=
 =?us-ascii?Q?T3kWdAjArQFlydF+jKbVMPdO0SPzX7/vPYhcux+q04V3R/McqFkHbUbyP0Zh?=
 =?us-ascii?Q?ICb4I/u6+hqvYVfBL5Q1WqPkUqnNRPkukCMQNq5leZWgEoDA+nM6X0QVfT+c?=
 =?us-ascii?Q?fm3FtsDZt3misJEjT7nmHSdLyel/8QwIEr4H9JA0swIuJpbD8xtLN60/VVyZ?=
 =?us-ascii?Q?d/6IIl2eKiq3SieZPsmu9oCIQJgLNiiCfJgMsb72OzOWju+pgVjnoDikSJXJ?=
 =?us-ascii?Q?jxgvBnwWbsifzRpXN4psu0YQH6ezZo9auQpvnQALRC02ZVbpkr8EUM76stWt?=
 =?us-ascii?Q?JYFu3cAQndc0QunYNxQOXHCSmCD1bKeJPBTNhNuBGkHwqwerUybReo0SMbsZ?=
 =?us-ascii?Q?W9Lp1aJon17UHwmDdDFGSonQkrhL2vrek/nv4fa/rGpRnwXBq7kqTuqfE8OL?=
 =?us-ascii?Q?GJdxYR3aZA31bFpsS1RZzydQfkWAZbqzbjxmdQSgcfS8PND6bpxvosuJ0qgf?=
 =?us-ascii?Q?+N19ktz+A91eKnpRJVKydP1tdDOH5aPm+tB8eeHkVF4IOMBhCfSF8ZZmKmve?=
 =?us-ascii?Q?hmh5wK+9vCeci4lt8/qNJty+AMV5KRvjEIM099vENBw+Uh0v48V6u6WqTX5W?=
 =?us-ascii?Q?JCKfIXpIdfa4TFTmGlU6jS91kOrquQcA7BWXzwotVh+ECr4AD73lMm2TQrtG?=
 =?us-ascii?Q?4e/QSfEmGa2yf14BYGCWxh49ct3+JNqLh+eT+uznII/binwbd9Y/uMtbNw3G?=
 =?us-ascii?Q?LjFf6Lpb5T/LHDUtMyWE/gSuMkSCsB3l44XGEa7D9AikpnkQfvVa4bryRXdd?=
 =?us-ascii?Q?1fe8fg7eUXsXV78SepdxJajqCjL7JrzW/Q7tJ1FZqXu6H+cn9Z+TvKOEGSKg?=
 =?us-ascii?Q?HnpDGT4LdTte6UCgzYYIIkYf3Lm6c34yJkaPhue+6DRBocJwDW81CgsBybcO?=
 =?us-ascii?Q?7ng214yX+82DdGK7EXU6mw7ZQFag+pKX0GZ7cptci7bE78UVjQR9awPR/AIV?=
 =?us-ascii?Q?ndCXEtLFIANWCNFSiG3F0P5/bjEqr50UAV1g4ID6c1d6uvrHKqtXjtlzNMMB?=
 =?us-ascii?Q?TyohDUTZsrLsylPGOsRQfSQn8CkRrAy79yxi3uqWLpUfVO84CRHwUVSfHv5p?=
 =?us-ascii?Q?kYCproz7vlBy+X1kinUi7Y8G7+g68uEQ2w26qOYM2B6XsIATXpMtmvlx/KXc?=
 =?us-ascii?Q?BjjrRNGRtpwHvolG1bkZgldJhhG92Hk3emaH5EkwvHGjXp4rm/PsMjyC/txU?=
 =?us-ascii?Q?dXWmBaJ1UucIvcUZTNzcxWnTg2nuedGH17OqF7mV3hPUgCeaiFqJizZ0OtWa?=
 =?us-ascii?Q?cKCcgz10WoVK1PWGjl4KIzOr9bw0Z3Pmz28tmksb1GnMz7FnapxOVGj7Pt82?=
 =?us-ascii?Q?MEfGp79L5VUJrR80Men4K7doA7LTwCMPbTb7a+kyWgomt7UgSaZUsvzQLutR?=
 =?us-ascii?Q?T7Hr5VrykZaV3V0dQGDR/SUw8iBT4DDHqoh0SJuOVOZFDdeJNj/ij8w9ZADH?=
 =?us-ascii?Q?NKnvWAsOBIVAb9X7wq0j8NdIWc4HR0SLJLQ8n4TQhi7ZohD1HqzfMoY9pz4p?=
 =?us-ascii?Q?K/6Uh+Akribu3dK+R4AMx4aQM9YnU0Nr0u2hsWBhuNTfiO64idBbzewBCAoZ?=
 =?us-ascii?Q?j2cNALN+nIIFl476nPCCJLTGaQuRop3HLL2ra3lmKC42jG9DXe69PTDsmAXt?=
 =?us-ascii?Q?TMGlAg=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3bfa2e81-926b-4bab-2039-08d9d41a0ec1
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jan 2022 09:17:43.3286
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: hZIAQTh/sSzwgnE/RtveKfzup7UWA2Obta+80ucie+LFR32B20SAK/OXMWO977txUZXk6jwvfuxwABX90GC78lm3qqAs3S+a1bvBtnDI9Dw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR10MB1422
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10222 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 suspectscore=0 spamscore=0
 mlxlogscore=999 adultscore=0 phishscore=0 malwarescore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2201100065
X-Proofpoint-GUID: 1q3VZ7ghbXfNWyy4q-c1k7aEWoI0Go5y
X-Proofpoint-ORIG-GUID: 1q3VZ7ghbXfNWyy4q-c1k7aEWoI0Go5y
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Seems reasonable.

On Sun, Jan 09, 2022 at 11:26:52AM +1300, Paulo Miguel Almeida wrote:
> meta-comments:
> - I'm not entirely sure if -EPERM is the best error code to return here,
>   I'm taking suggestions

Better to just return -EINVAL.

> ---
>  drivers/staging/pi433/pi433_if.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index 051c9052aeeb..3e9913f4bc7d 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -108,6 +108,9 @@ struct pi433_device {
>  struct pi433_instance {
>  	struct pi433_device	*device;
>  	struct pi433_tx_cfg	tx_cfg;
> +
> +	/* control flags */
> +	bool			tx_cfg_initialized;
>  };
>  
>  /*-------------------------------------------------------------------------*/
> @@ -823,6 +826,16 @@ pi433_write(struct file *filp, const char __user *buf,
>  	if (count > MAX_MSG_SIZE)
>  		return -EMSGSIZE;
>  
> +	/*
> +	 * check if tx_cfg has been initialized otherwise we won't be able to
> +	 * config the RF trasmitter correctly due to invalid settings
> +	 */
> +	if (!instance->tx_cfg_initialized) {
> +		dev_dbg(device->dev,
> +			"write: failed due to uninitialized tx_cfg");

Use dev_notice_once() or similar.  Maybe "unconfigured" instead of
uninitialized?

	dev_notice_once(device->dev,
			"write: failed due to unconfigured tx_cfg (see PI433_IOC_WR_TX_CFG)");


> +		return -EPERM;
> +	}
> +
>  	/*
>  	 * write the following sequence into fifo:
>  	 * - tx_cfg
> @@ -897,6 +910,7 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  			return -EFAULT;
>  		mutex_lock(&device->tx_fifo_lock);
>  		memcpy(&instance->tx_cfg, &tx_cfg, sizeof(struct pi433_tx_cfg));
> +		instance->tx_cfg_initialized = true;
>  		mutex_unlock(&device->tx_fifo_lock);
>  		break;
>  	case PI433_IOC_RD_RX_CFG:
> @@ -950,7 +964,7 @@ static int pi433_open(struct inode *inode, struct file *filp)
>  	/* setup instance data*/
>  	instance->device = device;
>  	instance->tx_cfg.bit_rate = 4711;

This is sort of pointless because it can't work until that gets over
written.


> -	// TODO: fill instance->tx_cfg;
> +	instance->tx_cfg_initialized = false;

kzalloc() will already set this to false.

regards,
dan carpenter