[PATCH 1/1][MTD] Adding IOCTLs for run-time partitioning support

Artem Bityutskiy dedekind1 at gmail.com
Mon Aug 23 10:33:46 EDT 2010

On Thu, 2010-07-29 at 17:00 +0530, Rohit Hassan Sathyanarayan wrote:
> Hi All
> Sending the patch on MTD for adding four IOCTLs.
> Signed-off-by: Rohit HS <rohit.hs at samsung.com>

Could you pleas make scripts/checkpatch.pl happy?

Could you please take a look at the solution from Roman and compare it
to your solution: what is the difference, which one is better? He
submitted the patch earlier than you. And I may be mistaken, but your
solution looks broken, but I did not really look deep enough. See below.

Could you please also take a look at the discussion of Roman's patch and
the request for the interface - Arn asked to have it look as much as
possible as the corresponding ioctl for block devices.

I did not really review your patch, but few things are below.

> +
> +		{
> +		struct mtd_partition *pst_minfo = NULL;
> +		struct mtd_partition *pst_minfo_hold = NULL;
> +		struct mtd_partition st_free;
> +		uint64_t u64_userfreeoffset = 0;
> +		uint64_t u64_userfreesize = 0;

Do not prefix variable names with types - kernel people hate this, and
they will hate your patches if you do like this!

General suggestion: when you modify an existing file, stick to the style
used in this file, do not push for your own style. Or change the style
of the whole file.

> +		struct user_mtd *puser_partitions = NULL;
> +		struct user_mtd_info upartition;
> +		int i;

Variable names are too long and cryptic, try to think about s

> +			add_mtd_partitions(mtd, pst_minfo_hold,
> +						upartition.num_partitions);

So 'pst_minfo_hold' (bad name!) becomes the master partition. You
allocate it in this function. But who delets it? When? Could you please
point to the code which kfree()s this object?

Best Regards,
Artem Bityutskiy (Артём Битюцкий)

More information about the linux-mtd mailing list