Re: [PATCH] ipwireless: driver for 3G PC Card

["Stephen Blackheath [to Foxconn]" <stephen@symmetric.co.nz>]

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