Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 19 Sep 2022 12:36:05 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Nam Cao <namcaov@gmail.com>
Cc:     forest@alittletooquiet.net, gregkh@linuxfoundation.org,
        philipp.g.hortmann@gmail.com, linux-kernel@vger.kernel.org,
        linux-staging@lists.linux.dev
Subject: Re: [RFC PATCH 3/5] staging: vt6655: split device_alloc_rx_buf
Message-ID: <Yyg4BTymed1fEzQx@kadam>
References: <cover.1663273218.git.namcaov@gmail.com>
 <311eaa7dabe12d0baeb0af6f85c2b43b20b230a3.1663273218.git.namcaov@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <311eaa7dabe12d0baeb0af6f85c2b43b20b230a3.1663273218.git.namcaov@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?kCMVLkRfIhgOVQVSTCQBYOc8LptieN5fXU4Iafofq5uIJzS6ejp3lX+YlNdC?=
 =?us-ascii?Q?Vvt0aBX3WsSZlsLYjNI/7g8upxaVsRoyx5qPJm875r5/ayhJ1cb8CxyS3faW?=
 =?us-ascii?Q?TyrDTn3dSRRStKgJlV//3LHCvMR6RzedHQIIMkG6pIGsuv8AUsjZvGOUkPao?=
 =?us-ascii?Q?1KCDjYwQ19JKgNff0KxHcpKGoJBZ/BmeMsHAWoKS8D6Gqb0MMbzm5dfZUVRW?=
 =?us-ascii?Q?9Kgb9WBRskpnbyto+U7diKwme0LQVxf95/kL1Bp2demWzGsrsJgH1wGiZ6IT?=
 =?us-ascii?Q?tcjPbQ7rRt7XWZm1HRIABpLkKfE8Gdp7R52h4pkVkQLFLzfpsHgIvSvt9I2e?=
 =?us-ascii?Q?JImi5IMRtJpw0Q/Hrd7oOQJXffsDfDigNPEgy9OUgxG55OPFpdCzJ26DqH2Q?=
 =?us-ascii?Q?Qq/xzvFKjoAzotqhyM+WF69NQE3puAqhe9P4RUbCOmbu7FEVO6aqdswQe4Cm?=
 =?us-ascii?Q?yGwQWMFoPFuzGbm8TuZAaKvc6qlWj18uZeUho6uTFRlFXhzEqbemqV48anjw?=
 =?us-ascii?Q?9GXFTNdA+6RD0yobmg2JwGQMLmBNsz122p12NefY3fjTwieqkiC6Ey/FeO0P?=
 =?us-ascii?Q?MbDW5DHns0u5HQ1H7cxIZLpvQT/hIlhruOVrBmpMgUXBtUauktzUxvM/jXxt?=
 =?us-ascii?Q?AWLGLb20tySImReVNPdLQs+8iDFeEdFIWa1dAza5c33h8Nwd9aQgJiIB8Fek?=
 =?us-ascii?Q?rVt4RLHy9HFfFKQnmC3UvzX7JN6cN7X3c0q1JGslg9tLVTOuqUKS+gUEUKx4?=
 =?us-ascii?Q?IiBm15/iMN9dhROBbu1pN/J6DNZhqKdhoI2V9eUjj5JnnLd5g0mWL0kg42Ey?=
 =?us-ascii?Q?wArhNCHPIW7a2b7pOxaSe24Ncxx5xCv0OkYuh/vtDf7PpxLr6HvOcG5tEyc/?=
 =?us-ascii?Q?GlKogo4RsDfQgfz8tkR3YnNNx06FDPV9S7rYTmojHll1TXX+Me3DvT5t7DQG?=
 =?us-ascii?Q?qSQcN45ZYFlO6n5swFZc89dnObVhWHCydkypXX+Rt+hbnFdHnVtcgagCrbtR?=
 =?us-ascii?Q?WvTsvSMpCQpbfnp6idtS3cXCUGIfPHaJGy9Sz+lX+jt2w+dtFLGUkDNAAaql?=
 =?us-ascii?Q?oOL537Pmm2k++vvzyNoSdE+Oe24uEe9jcoGzOadIEe3GGhNDLJgGUf72igtT?=
 =?us-ascii?Q?dcsqiaffh7hzQGFcxdbCcfWwh4olCRjz+OHC/ffDrBhQqGl/nF4p13WkKWHz?=
 =?us-ascii?Q?SDJ8pDIcu0HkltHky5WtButQRuZshkfN7nHRSIfveSwWUsuATXkaoisn23gp?=
 =?us-ascii?Q?NkEpj3i4JvJMvEpQcm1FJv/nxeQ/8bwIzq8pnRgwGMC8tde9gBY7ZWhHPJyV?=
 =?us-ascii?Q?rpZxidBU+56XCgPmunwuc7L65GElWEqRIFsIpfHMJW/Ry3fdoyYqh1pS3G/r?=
 =?us-ascii?Q?2Y25npbdG4/QRg0OEyqy9+94pAcEJXNmUrjpnSAAh5lHwjOdKVocrnK4HXPk?=
 =?us-ascii?Q?Gn33PdRVFJ4qVji/NKDFv9quMpqFYMxfY4dQNlsdLyhkmCjPoP/ZbmoA2wWS?=
 =?us-ascii?Q?TdoYrhQ6Kl0S3ddoQM6e2cjpwEJtwkdy0+cWOHsN5IRWFzses+rYvLxw+vW3?=
 =?us-ascii?Q?rjIy9scFaWYAy8xEo+S51wbIYwukjjWInsELwvEBZAHfrvkg0nICBHmwPhDI?=
 =?us-ascii?Q?iA=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 27adee37-e191-43e7-1308-08da9a226e85
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Sep 2022 09:36:30.4345
 (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: xJDtksVKrcaxIJYCFNdxWz/udMnsubo3QK6uEZD0lp9aARB5XXeQc+50F8PBZZVndGJ0VVOR/SL63zgflI21o/zS9+OIscy784CP8oFMbFQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4437
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-19_05,2022-09-16_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 phishscore=0 spamscore=0
 bulkscore=0 mlxscore=0 malwarescore=0 suspectscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2209130000
 definitions=main-2209190064
X-Proofpoint-GUID: m4Su0JIbQEVq-E4raEm7osLUcbE2CdmV
X-Proofpoint-ORIG-GUID: m4Su0JIbQEVq-E4raEm7osLUcbE2CdmV
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Sep 15, 2022 at 10:29:34PM +0200, Nam Cao wrote:
> The function device_alloc_rx_buf does 2 things: allocating rx buffer
> and initializing the rx descriptor's values. Split this function into
> two, with each does one job.
> 
> This split is preparation for implementing correct out-of-memory error
> handling.
> 
> Signed-off-by: Nam Cao <namcaov@gmail.com>
> ---
>  drivers/staging/vt6655/device_main.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
> index 79325a693857..27fe28156257 100644
> --- a/drivers/staging/vt6655/device_main.c
> +++ b/drivers/staging/vt6655/device_main.c
> @@ -133,6 +133,7 @@ static int device_init_td1_ring(struct vnt_private *priv);
>  static int  device_rx_srv(struct vnt_private *priv, unsigned int idx);
>  static int  device_tx_srv(struct vnt_private *priv, unsigned int idx);
>  static bool device_alloc_rx_buf(struct vnt_private *, struct vnt_rx_desc *);
> +static void device_init_rx_desc(struct vnt_private *priv, struct vnt_rx_desc *rd);
>  static void device_free_rx_buf(struct vnt_private *priv,
>  			       struct vnt_rx_desc *rd);
>  static void device_init_registers(struct vnt_private *priv);
> @@ -615,6 +616,8 @@ static int device_init_rd0_ring(struct vnt_private *priv)
>  			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
>  			ret = -ENOMEM;
>  			goto err_free_rd;
> +		} else {
> +			device_init_rx_desc(priv, desc);
>  		}

None of these else statements make sense.  It should be:

		ret = -ENOMEM;
		goto err_free_rd;
	}

	device_init_rx_desc(priv, desc);
	desc->next = &priv->aRD0Ring[(i + 1) % priv->opts.rx_descs0];

I haven't reviewed the patch totally.  I don't understand why it's doing
this here instead of at the end.  But then I don't understand why it
needs to be in a separate function at all.

This patch does not make sense.  The commit description says that this
is a "preparation" patch.  Maybe fold it in with patch 5?  The rule is
"one thing per patch" not "half a thing per patch".

regards,
dan carpenter