linux-kernel - Re: [PATCH] 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]

Date:	Thu, 31 Jan 2008 10:29:30 +1300
From:	"Stephen Blackheath [to Foxconn]" <stephen@...metric.co.nz>
To:	Pavel Machek <pavel@....cz>
CC:	David Sterba <dsterba@...e.cz>, torvalds@...ux-foundation.org,
	linux-kernel@...r.kernel.org, jkosina@...e.cz, benm@...metric.co.nz
Subject: Re: [PATCH] ipwireless: driver for 3G PC Card

Pavel & all,


Pavel Machek wrote:

>> +/* I/O ports and bit definitions for version 2 of the hardware */
>> +
>> +struct MEMCCR {
>> +	unsigned short PCCOR;		/* Configuration Option Register */
>> +	unsigned short PCCSR;		/* Configuration and Status Register */
>> +	unsigned short PCPRR;		/* Pin Replacemant Register */
>> +	unsigned short PCSCR;		/* Socket and Copy Register */
>> +	unsigned short PCESR;		/* Extendend Status Register */
>> +	unsigned short PCIOB;		/* I/O Base Register */
>> +};
>>     
>
> Could we get better names? PCIOB is cryptic, pci_io_base is pretty
> good.
>
>   
We should keep these names because they are part of the interface
between host and card defined by the manufacturer.
>> +static irqreturn_t ipwireless_handle_v1_interrupt(int irq,
>> +						  struct ipw_hardware *hw)
>> +{
>> +	unsigned short irqn;
>> +	unsigned short ack;
>> +
>> +	irqn = inw(hw->base_port + IOIR);
>> +
>> +	/* Check if card is present */
>> +	if (irqn == (unsigned short) 0xFFFF) {
>> +		if (++hw->bad_interrupt_count >= 100) {
>> +			/*
>> +			 * It is necessary to disable the interrupt at this
>> +			 * point, or the kernel hangs, interrupting repeatedly
>> +			 * forever.
>> +			 */
>> +			hw->irq = irq;
>> +			hw->removed = 1;
>> +			disable_irq_nosync(irq);
>> +			printk(KERN_DEBUG IPWIRELESS_PCCARD_NAME
>> +					": Mr. Fluffy is not happy!\n");
>> +		}
>> +		return IRQ_HANDLED;
>>     
>
> Not sure how this is supposed to work. If you assume unshared
> interrupts, it should be possible to return something and make core
> care.
>
> If you are assuming shared interrupts, either you should disable on
> first 0xFFFF (are you sure cast is needed, btw?), or not at all,
> because it could be the other device sedning you 100 of those...
>
> ...so which one is it?
>   
This code is obsolete (a workaround to an embedded system bug) and
should be removed - sorry I didn't step on it earlier.  It can removed
with minimal risk of destabilizing the driver.  It should look like this:

static irqreturn_t ipwireless_handle_v1_interrupt(int irq,
ipw_hardware_t *hw)
{
        u_short irqn;
        u_short ack;

        irqn = inw(hw->base_port + IOIR);

        if (irqn == (u_short) 0xFFFF)
                return IRQ_NONE;
        else if (irqn != 0) {
                ack = 0;
                /* Transmit complete. */
                if (irqn & IR_TXINTR) {
                        hw->tx_ready++;
                        ack |= IR_TXINTR;
                }

                /* Received data */
                if (irqn & IR_RXINTR) {
                        ack |= IR_RXINTR;
                        hw->rx_ready++;
                }
                if (ack != 0) {
                        outw(ack, hw->base_port + IOIR);

                        /* Perform the I/O retrieval in a tasklet,
because the ppp_generic
                           may be called from a tasklet, but not from a
hardware interrupt. */
                        if (!hw->tasklet_pending) {
                                hw->tasklet_pending = 1;
                                tasklet_schedule(&hw->tasklet);
                        }
                }
                return IRQ_HANDLED;
        } else
                return IRQ_NONE;

}

The v2_v3 handler should not have it either, and should start like this:

static irqreturn_t ipwireless_handle_v2_v3_interrupt(int irq,
ipw_hardware_t *hw)
{
    int tx = 0;
    int rx = 0;
    int rx_repeat = 0;
    int b_try_MemTX_OLD;
    do {
        u_short memtx = ioread16(hw->MemTX);
        u_short memtx_serial;
        u_short memrxdone = ioread16(&hw->memInfReg->MemRXDone);

        b_try_MemTX_OLD = 0;

        /* check whether the interrupt was generated by ipwireless card */
        if (!(memtx & MEMTX_TX) && !(memrxdone & MEMRX_RX_DONE))
            return IRQ_NONE;

        /* See if the card is physically present. Note that while it is
         * powering up, it appears not to be present. */
        if (ioread32(&hw->memInfReg->MemCardPresent) !=
CARD_PRESENT_VALUE) {
            u_short csr = ioread16(&hw->memCCR->PCCSR);
            csr &= 0xfffd;
            iowrite16(csr, &hw->memCCR->PCCSR);
            return IRQ_HANDLED;
        }

> Is some locking needed around *hw?
>   
I don't think so, but I'm happy to be corrected.


Steve


--
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/