linux-kernel - Re: [PATCH][v4] ipwireless: driver for 3G PC Card

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200802061215.58688.dsterba@suse.cz>
Date:	Wed, 6 Feb 2008 12:15:58 +0100
From:	David Sterba <dsterba@...e.cz>
To:	Marcel Holtmann <marcel@...tmann.org>
Cc:	torvalds@...ux-foundation.org, linux-kernel@...r.kernel.org,
	jkosina@...e.cz, benm@...metric.co.nz, stephen@...metric.co.nz
Subject: Re: [PATCH][v4] ipwireless: driver for 3G PC Card

Hi,

> > +irqreturn_t ipwireless_interrupt(int irq, void *dev_id, struct pt_regs
> > *regs) +{
> > +	struct ipw_hardware *hw = dev_id;
> > +
> > +	if (hw->hw_version == HW_VERSION_1)
> > +		return ipwireless_handle_v1_interrupt(irq, hw);
> > +	else
> > +		return ipwireless_handle_v2_v3_interrupt(irq, hw);
> > +}
>
> why is this not static? I think the interrupt routine should be in the
> file where you actually claim the interrupt.

This is due to current separation into files. hardware.c has many functions 
for handler bodies, while main.c registers the pcmcia structs in module init 
(and takes address of ipwireless_interrupt()). Long ago, the driver was one 
large file, and I don't want to merge the files back just to make some funcs 
static (and make the sources less readable).

>
> > +/*
> > + * Associate the specified network with this hardware, so it will
> > receive events + * from it.
> > + */
> > +void ipwireless_associate_network(struct ipw_hardware *hw,
> > +				  struct ipw_network *network)
> > +{
> > +	hw->network = network;
> > +}
>
> I think a #define would be simpler in this case.

Same reason, the macro would be used in a file where the struct is not yet 
defined.

> > +module_param(tty_major, int, 0);
> > +module_param_named(debug, ipwireless_debug, int, 0);
> > +module_param_named(loopback, ipwireless_loopback, int, 0);
> > +module_param_named(out_queue, ipwireless_out_queue, int, 0);
> > +MODULE_PARM_DESC(tty_major, "ttyIPWp major number [0]");
>
> Why is allowing to change the major still needed. I think we passed the
> bridge of the need for static numbers a long long long time ago.

Sounds reasonable. I personally haven't used this param (besides testing the 
code) and don't know of any related problems.

> > +static const char drv_name[] = IPWIRELESS_PCCARD_NAME;
>
> This looks useless to. Do we need that?

Removed. Used only in one place.

Thanks for comments. Updated version 5 will follow.

Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/